Add persistent storage to agent #131

borod108 · 2025-01-22T07:21:45Z

Add persistent storage to the agent and move the credentials and agent-id there.

machacekondra · 2025-01-22T08:40:54Z

cmd/planner-agent/main.go

+		return string(bytes.TrimSpace(content)), nil
+	}
+
+	fmt.Println("datadir: ", a.config.DataDir)


Please use logging instead of fmt.Println

machacekondra · 2025-01-22T08:49:23Z

data/ignition.template


 storage:
+  filesystems:
+    - path: /var/lib/data
+      device: /dev/sda


I think it's safe for now, but maybe in future we should improve it to use ID?

good point, future improvement!

tupyy · 2025-01-22T08:52:51Z

cmd/planner-agent/main.go

@@ -77,9 +76,9 @@ func (a *agentCmd) Execute() error {
 	undo := zap.ReplaceGlobals(logger)
 	defer undo()

-	agentID, err := a.readFile(agentFilename)
+	agentID, err := a.readPersistentFile(agentFilename)


why change of name here? what value Persistent bring here?

why not adding changing the initial method and just add a parameter with the folder, or better just path the whole path and do the join here

There is some code duplication here indeed, let me think how to refactor - obviously a common underlying function with a param for basedir, but I still want two separate names to call it, so that I do not need to think about the right config option, just know if I am reading from the persistent storage or the volatile one,

tupyy · 2025-01-22T08:53:05Z

cmd/planner-agent/main.go

 	if err != nil {
-		zap.S().Fatalf("failed to retreive agent_id: %v", err)
+		zap.S().Fatalf("failed to retrieve agent_id: %v", err)


in another PR

:/ I thought I will sneak this one line change which is in the execution path of what I am working on - but ok.

machacekondra · 2025-01-22T08:56:12Z

internal/image/ova.go

@@ -179,6 +187,20 @@ func (o *Ova) ovfSize() (int, error) {
 	return calculateTarSize(int(fileInfo.Size())), nil
 }

+func (o *Ova) diskSize() (int, error) {
+	file, err := os.Open("data/persistence-disk.vmdk")


I just wonder if we really need to bring the disk here, could not user create it when deploying OVA?

Can't we just leave the option for the user? if he decides to add a disk we move data folder there if not we proceed as planned?

I am not sure what the benefit will be? currently it is a lazy provisioned very small file, it is now part of the OVA and will be deployed on the same storage selected by the user in the process of installation.
There is a higher complexity in allowing the user to add a disk - we do not know if it is sata or iscsi, not sure of it's id or name. What if the user wants to add a disk for a different purpose and wipes it so we loose the credentials and the id? Do we require that he adds a specific label? what if they remove the disk later (it is their disk...).
I think, at least for persistence of critical elements that like credentials this is a better solution, but I am open to be convinced.

Yep, makes sense. I wasn't just sure if in the process of deployment user still need to select the disk for another VMDK, but I understand he don't have to and he pick the storage just once for both, which is great. So let's do it your way for sure.

I wasn't sure how the deployment process will be. If we automate the disk it's fine then.

just a remark: we never lose the id. the agent_id is created when agent service starts.

tupyy · 2025-01-22T09:01:40Z

Is this optional for the user? Why not move data folder on the new disk and avoid refactoring of the code.
@machacekondra

machacekondra · 2025-01-22T10:15:57Z

Is this optional for the user? Why not move data folder on the new disk and avoid refactoring of the code. @machacekondra

Well, right, yeah I think that all the data could now be stored on the disk and be persistent, unless I miss something.

borod108 · 2025-01-22T15:04:56Z

Is this optional for the user? Why not move data folder on the new disk and avoid refactoring of the code. @machacekondra

Well, right, yeah I think that all the data could now be stored on the disk and be persistent, unless I miss something.

Well my thought was that we want to keep the disk minimal and the data collected may be large, and that on reset we want to restart the collection process and the sending of the inventory, right?

tupyy · 2025-01-22T15:46:12Z

Is this optional for the user? Why not move data folder on the new disk and avoid refactoring of the code. @machacekondra

Well, right, yeah I think that all the data could now be stored on the disk and be persistent, unless I miss something.

Well my thought was that we want to keep the disk minimal and the data collected may be large, and that on reset we want to restart the collection process and the sending of the inventory, right?

will the disk be smaller than 4gb? cause if not on the disk than you fill up the memory.
the user cannot know this "restart" behavior. In the future, I would expect to collect data anyway. not just once. What's the point on keep running the agent if we don't collect anything?

borod108 · 2025-01-23T10:24:56Z

Is this optional for the user? Why not move data folder on the new disk and avoid refactoring of the code. @machacekondra

Well, right, yeah I think that all the data could now be stored on the disk and be persistent, unless I miss something.

Well my thought was that we want to keep the disk minimal and the data collected may be large, and that on reset we want to restart the collection process and the sending of the inventory, right?

will the disk be smaller than 4gb? cause if not on the disk than you fill up the memory. the user cannot know this "restart" behavior. In the future, I would expect to collect data anyway. not just once. What's the point on keep running the agent if we don't collect anything?

It is currently 50mb, could also be 5 actually for what we are doing now. Not sure what "user cannot know this restart behavior?" means exactly. The point of this entire thing is to improve restart behavior, which is our current solution to restart collection.

I do fully agree we should have collection for a running agent, we do not have it now and it is not the goal of this PR to allow it, do we have a timeline for this?

machacekondra · 2025-01-23T12:56:39Z

Well the truth is that currently the only option to collect the inventory again is to restart the VM, unless user has access to VM via SSH and restart the Agent. We don't have any option to re-collect data via button or something, which we of course can add ;)
But let's for now persist only a credentials so we will say user can re-collect data via VM restart.

app-sre-bot · 2025-01-23T12:56:43Z

Can one of the admins verify this patch?

machacekondra · 2025-01-23T13:11:57Z

@borod108 Can u please fix the tests?

machacekondra · 2025-02-04T08:31:48Z

.github/workflows/kind.yml

          kubectl wait --for=condition=Ready pods --all --timeout=240s
          kubectl port-forward --address 0.0.0.0 service/migration-planner-agent 7443:7443 &
          kubectl port-forward --address 0.0.0.0 service/migration-planner 3443:3443 &

      - name: Run test
        run: |
+          mkdir /tmp/untarova
+          cp data/persistence-disk.qcow2 /tmp/untarova/persistence-disk.qcow2


Can we create this file on the fly instead of storing in git please?

Add persistent storage to the agent and move the credentials and agent-id there. Signed-off-by: borod108 <[email protected]>

machacekondra · 2025-02-04T19:30:29Z

If we would have an e2e test to test VM reboot in future it would be cool ;) But let's wait for single source.

machacekondra · 2025-02-04T21:13:51Z

@tupyy @nirarg PTAL

nirarg

Looks good

borod108 force-pushed the task/unique-vm-id branch from b9cf0fa to 481db17 Compare January 22, 2025 07:23

machacekondra requested a review from tupyy January 22, 2025 08:23

machacekondra reviewed Jan 22, 2025

View reviewed changes

tupyy reviewed Jan 22, 2025

View reviewed changes

machacekondra reviewed Jan 22, 2025

View reviewed changes

borod108 force-pushed the task/unique-vm-id branch 2 times, most recently from c528f1a to 469c025 Compare February 3, 2025 21:59

machacekondra reviewed Feb 4, 2025

View reviewed changes

Add persistent storage to agent

fc8244b

Add persistent storage to the agent and move the credentials and agent-id there. Signed-off-by: borod108 <[email protected]>

borod108 force-pushed the task/unique-vm-id branch from 469c025 to fc8244b Compare February 4, 2025 14:27

machacekondra approved these changes Feb 4, 2025

View reviewed changes

nirarg approved these changes Feb 5, 2025

View reviewed changes

tupyy approved these changes Feb 5, 2025

View reviewed changes

borod108 merged commit 405bac1 into kubev2v:main Feb 6, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add persistent storage to agent #131

Add persistent storage to agent #131

borod108 commented Jan 22, 2025

machacekondra Jan 22, 2025

borod108 Jan 22, 2025

machacekondra Jan 22, 2025

borod108 Jan 22, 2025

tupyy Jan 22, 2025

tupyy Jan 22, 2025

borod108 Jan 22, 2025

tupyy Jan 22, 2025

borod108 Jan 22, 2025 •

edited

Loading

machacekondra Jan 22, 2025

tupyy Jan 22, 2025

borod108 Jan 22, 2025

machacekondra Jan 22, 2025

tupyy Jan 22, 2025

tupyy commented Jan 22, 2025

machacekondra commented Jan 22, 2025

borod108 commented Jan 22, 2025 •

edited

Loading

tupyy commented Jan 22, 2025

borod108 commented Jan 23, 2025

machacekondra commented Jan 23, 2025 •

edited

Loading

app-sre-bot commented Jan 23, 2025

machacekondra commented Jan 23, 2025

machacekondra Feb 4, 2025

borod108 Feb 4, 2025

machacekondra commented Feb 4, 2025

machacekondra commented Feb 4, 2025

nirarg left a comment

Add persistent storage to agent #131

Add persistent storage to agent #131

Conversation

borod108 commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borod108 Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tupyy commented Jan 22, 2025

machacekondra commented Jan 22, 2025

borod108 commented Jan 22, 2025 • edited Loading

tupyy commented Jan 22, 2025

borod108 commented Jan 23, 2025

machacekondra commented Jan 23, 2025 • edited Loading

app-sre-bot commented Jan 23, 2025

machacekondra commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machacekondra commented Feb 4, 2025

machacekondra commented Feb 4, 2025

nirarg left a comment

Choose a reason for hiding this comment

borod108 Jan 22, 2025 •

edited

Loading

borod108 commented Jan 22, 2025 •

edited

Loading

machacekondra commented Jan 23, 2025 •

edited

Loading