Bring elemental-cli code into the agent #13

Itxaka · 2023-05-01T09:22:19Z

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

codecov-commenter · 2023-05-01T09:32:43Z

Codecov Report

Merging #13 (a749f66) into main (e3c52e5) will increase coverage by 39.25%.
The diff coverage is 71.52%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##             main      #13       +/-   ##
===========================================
+ Coverage   22.10%   61.36%   +39.25%     
===========================================
  Files          17       50       +33     
  Lines        1330     6038     +4708     
===========================================
+ Hits          294     3705     +3411     
- Misses        983     2010     +1027     
- Partials       53      323      +270

Impacted Files	Coverage Δ
internal/agent/install.go	`4.91% <0.00%> (-0.20%)`	⬇️
internal/agent/reset.go	`0.00% <0.00%> (ø)`
internal/agent/upgrade.go	`0.00% <0.00%> (ø)`
pkg/live/common.go	`0.00% <0.00%> (ø)`
pkg/types/v1/errors.go	`0.00% <0.00%> (ø)`
pkg/cloudinit/console.go	`40.62% <40.62%> (ø)`
pkg/utils/getpartitions.go	`44.44% <44.44%> (ø)`
pkg/types/v1/syscall.go	`50.00% <50.00%> (ø)`
pkg/types/v1/logger.go	`51.47% <51.47%> (ø)`
pkg/partitioner/disk.go	`56.52% <56.52%> (ø)`
... and 26 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Brings the elemental-cli config generation methods so the installer reads the config files for elemental and merges them back into the final config. This has the side effect of bringing viper as a dep as we rely heavily on viper to unmarshall everything so maybe we should move the cli to use viper Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Itxaka · 2023-05-01T16:12:25Z

only install for now uses elemental libs directly integrated, seems to work as expected.
upgrade and reset will come later.

config merge will be a pita! elemental does a lot of stuff...

And still missing dropping all unneeded stuff for build...

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Available on commit 0c9d2bd9e6ae0a312ed46b441f00d93b93b79a24 Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Itxaka · 2023-05-02T14:52:11Z

upgrade+reset done

reset fails with a panic so not ready yet :D

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Itxaka · 2023-05-03T11:48:53Z

Reset Works! Upgrade Works!

jimmykarily · 2023-05-04T05:59:48Z

internal/agent/install.go

@@ -249,47 +251,29 @@ func Install(dir ...string) error {
 }

 func RunInstall(options map[string]string) error {
+	// TODO: run yip directly
 	utils.SH("elemental run-stage kairos-install.pre")             //nolint:errcheck


I thought the goal of this PR was to not have "elemental" in the image at all. Are we doing it in steps?

we can work on that afterwards in paralell with other work, no need to block this further with that

jimmykarily · 2023-05-04T06:07:47Z

internal/agent/install.go

+	_, reboot := options["reboot"]
+	_, poweroff := options["poweroff"]


Suggested change

_, reboot := options["reboot"]

_, poweroff := options["poweroff"]

reboot := options["reboot"]

poweroff := options["poweroff"]

this is a map[string]string], so cant get just that without more work afterwards. Doing it trhis way means that the reboot and poweroff are directly a bool type :)

could options["reboot"] be set to "false" ? Because in that case reboot would be true (because the key exists), although it's value is "false"

That is a good point!

welp, this actually does the same behaviour that the old one! so yeah, not touching this in the PR, maybe afterwards as its a behaviour change!

jimmykarily · 2023-05-04T06:14:06Z

internal/agent/install.go

+		return err
+	}
+
+	// Set our cloud-init to the file we just created


This comment belongs above line 306 below I think

jimmykarily · 2023-05-04T06:20:31Z

internal/agent/reset.go

 	bus.Manager.Response(sdk.EventBeforeReset, func(p *pluggable.Plugin, r *pluggable.EventResponse) {
-		err := json.Unmarshal([]byte(r.Data), &options)
+		err := json.Unmarshal([]byte(r.Data), &map[string]string{})


We used to unmarshal to options because we needed that variable later. What's the reason of unmarshaling now? Just to print the error if it's not "umarshable"?

options was an empty map[string]string{} before. This is just removing an extra var. No idea why it was done like this before, I guess because all install,reset,upgrade do the same? In case we add options later?

oooh I see, it comes from the r.Data.

Where the fuck does that r.Data magically appears from?

I have no idea where that comes from or how does it get set to be honest, I'll bring it up on the meeting today.

Also good catch, this was setting reset-persistent by default in no data was coming in, I missed that!

jimmykarily · 2023-05-04T06:26:21Z

internal/agent/reset.go


 	"github.com/mudler/go-pluggable"
 	"github.com/pterm/pterm"
 )

 func Reset(dir ...string) error {
+	// TODO: Enable args? No args for now so no possibility of reset persistent or overriding the source for the reset
+	// Nor the auto-reboot via cmd?


afaict --reset-persistent flag was set through r.Data in this code. Why can't we keep it this way?

This comment relates for the cli being called. If you call kairos-agent reset there is no flags that can be passed manually to override the default config. Im just thinking that maybe we should be able to override the config via flags.

jimmykarily · 2023-05-04T06:35:44Z

internal/agent/upgrade.go

+	}
+
+	// Override the default luet to pass the auth
+	// Remember to create the temp dir!


Is this a TODO note? Who should remember it and when?

Well this has history :D
Its just a note to mention that when creating a custom luet object you need to create a temp dir for it explicitely. Otherwise things go wrong very soon. I'll expand the comment a bit more!

But there is nothing to do in this part of the code no? Maybe this reminder belongs closer to where the temp dir should be created.

It should be created here, when you create the new luet. I hope to fix this in the future once the code is folder but currently if someone removes that line, unintended consequences will happen :D

jimmykarily · 2023-05-04T06:36:54Z

internal/agent/upgrade.go

+	resetConfig.Luet = l
+
+	// Generate the upgrade spec
+	resetSpec, err := elementalConfig.ReadUpgradeSpec(resetConfig)


This variable should probably be called upgradeSpec

wops! good catch!

jimmykarily · 2023-05-04T06:37:56Z

internal/agent/upgrade.go

-	)
-
+	// Load the upgrade Config from the system
+	resetConfig, err := elementalConfig.ReadConfigRun("/etc/elemental")


This variable should probably be called upgradeConfig (though it seems that we load the same config on reset so maybe a more generic name in both cases would be better)

indeed. It actually should be config but there is already like 32 different config vars lol. I'll change it!

Yes, we will hopefully consolidate configs here (That story is a can of worms...)

jimmykarily · 2023-05-04T06:48:41Z

pkg/action/build-iso.go

@@ -0,0 +1,287 @@
+/*


I don't think we should bring the build parts too. There is another story to move those to osbuilder (kairos-io/kairos#711). Until we do, I guess it's ok to keep them in the old "elemental-cli" binary, what do you think? Or are we planning to replace elemental-cli with kairos-agent first and then move the build parts to osbuilder?

yep yep, this is a "fast, dirty" job to bring the libs in here, there is still lot of work to clean this up and unify, but I didnt want to block this further. Removing those parts can be done afterwards in parallel with the config merge and others.

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

jimmykarily

I'm fine to merge this as it is and improve the code in future PRs. After all, this is mainly a code migration from another repository. No need to fix everything at once.

Bring elemental-cli code into the agent

95a7ba9

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Run the install directly

3f283f9

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Itxaka force-pushed the ele branch from 978aa33 to 3f283f9 Compare May 1, 2023 14:41

Itxaka force-pushed the ele branch from 9399f01 to 07cf7c3 Compare May 1, 2023 16:04

Itxaka added 7 commits May 1, 2023 18:15

Append to cloud-init paths

8c799f1

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Allow setting debug for install

408e0cc

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

fix cloud-init set on install

80f1319

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Run upgrade directly from agent instead of shelling out

e8b2b63

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Run reset directly without shelling out

579e659

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Drop debug

bf91c94

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Bump kairos-sdk for the os-release variables change

62ee2cf

Available on commit 0c9d2bd9e6ae0a312ed46b441f00d93b93b79a24 Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Exit with proper error if elemental-cli libs fail

6278889

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Itxaka requested a review from a team May 3, 2023 11:50

jimmykarily reviewed May 4, 2023

View reviewed changes

Itxaka added 2 commits May 4, 2023 09:22

Adress comments

a749f66

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Restore options in reset

641b608

Signed-off-by: Itxaka <itxaka.garcia@spectrocloud.com>

Itxaka requested review from jimmykarily and a team May 5, 2023 11:54

jimmykarily approved these changes May 5, 2023

View reviewed changes

Itxaka merged commit 002b8ba into main May 5, 2023

Itxaka deleted the ele branch May 5, 2023 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring elemental-cli code into the agent #13

Bring elemental-cli code into the agent #13

Itxaka commented May 1, 2023 •

edited by jimmykarily

Loading

codecov-commenter commented May 1, 2023 •

edited

Loading

Itxaka commented May 1, 2023

Itxaka commented May 2, 2023

Itxaka commented May 3, 2023

jimmykarily May 4, 2023

Itxaka May 4, 2023

jimmykarily May 4, 2023

Itxaka May 4, 2023

jimmykarily May 4, 2023

Itxaka May 5, 2023

Itxaka May 5, 2023

jimmykarily May 4, 2023

jimmykarily May 4, 2023

Itxaka May 4, 2023

Itxaka May 4, 2023

Itxaka May 4, 2023

jimmykarily May 4, 2023

Itxaka May 4, 2023

jimmykarily May 4, 2023

Itxaka May 4, 2023

jimmykarily May 4, 2023

Itxaka May 4, 2023

jimmykarily May 4, 2023

Itxaka May 4, 2023

jimmykarily May 4, 2023

Itxaka May 4, 2023

jimmykarily May 4, 2023

jimmykarily May 4, 2023

Itxaka May 4, 2023

jimmykarily left a comment

		_, reboot := options["reboot"]
		_, poweroff := options["poweroff"]

Bring elemental-cli code into the agent #13

Bring elemental-cli code into the agent #13

Conversation

Itxaka commented May 1, 2023 • edited by jimmykarily Loading

codecov-commenter commented May 1, 2023 • edited Loading

Codecov Report

Itxaka commented May 1, 2023

Itxaka commented May 2, 2023

Itxaka commented May 3, 2023

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

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

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

Choose a reason for hiding this comment

jimmykarily left a comment

Choose a reason for hiding this comment

Itxaka commented May 1, 2023 •

edited by jimmykarily

Loading

codecov-commenter commented May 1, 2023 •

edited

Loading