Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

elemental: bump elemental-cli to v0.1.0 #375

Merged
merged 2 commits into from
Dec 28, 2022
Merged

elemental: bump elemental-cli to v0.1.0 #375

merged 2 commits into from
Dec 28, 2022

Conversation

tjjh89017
Copy link
Contributor

@tjjh89017 tjjh89017 commented Nov 3, 2022 •
edited by bk201
Loading

We cannot use Elemental-cli InstallSpec struct to generate config.yaml. Because there are not all entry with "omitempty", it will cause generated config.yaml will have several empty entry but didn't be omitted.
Elemental-cli will treat those empty entry as user-specific value and override the default value.

Use ElementalInstallSpec to ensure that all entry will be "omitempty".

We must to use patched OS2 image to build iso
git clone -b elemental https://github.com/tjjh89017/os2
harvester/os2#51

Signed-off-by: Date Huang date.huang@suse.com

Related issue: harvester/harvester#2565

@tjjh89017 tjjh89017 force-pushed the elemental branch 2 times, most recently from 44682a9 to b9fec85 Compare November 3, 2022 09:19
@tjjh89017 tjjh89017 marked this pull request as ready for review November 8, 2022 06:40
@tjjh89017 tjjh89017 requested a review from bk201 November 8, 2022 06:40
@tjjh89017 tjjh89017 mentioned this pull request Nov 9, 2022
Closed
@tjjh89017 tjjh89017 changed the title WIP: elemental: bump elemental-cli to v0.1.0 elemental: bump elemental-cli to v0.1.0 Dec 5, 2022
starbops
starbops approved these changes Dec 8, 2022
View reviewed changes
Copy link
Member

@starbops starbops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one nit, the rest LGTM!

I also tested on single node installation, looks good. Thanks!

pkg/config/schemas_test.go
Comment on lines +21 to +22
ServerURL: "https://someserver:6443",
Token: "TOKEN_VALUE",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these two lines of spaces added unintentionally? Maybe we can remove them.

@tjjh89017 tjjh89017 mentioned this pull request Dec 13, 2022
Merged
@bk201 bk201 requested a review from masteryyh December 14, 2022 07:30
@bk201
Copy link
Member

bk201 commented Dec 14, 2022

Please help rebase, thanks.

@tjjh89017
Copy link
Contributor Author

rebase done.

@bk201
Copy link
Member

bk201 commented Dec 15, 2022

I have install.poweroff: true in my config file, and the flag seems not to be respected.
The node reboots after the installation rather than shutdown.

@tjjh89017
Copy link
Contributor Author

tjjh89017 commented Dec 15, 2022 •
edited
Loading

I have install.poweroff: true in my config file, and the flag seems not to be respected.
The node reboots after the installation rather than shutdown.

Could you try the harvester v1.1.1 also?
I think it should be harvester installer bug in go code part

poweroff flag will not be handle by elemntal but harvester installer directly

@bk201
Copy link
Member

bk201 commented Dec 15, 2022

The disk partition order changes as well. Can we stick to the previous one?
Old:

vda
vda1
vda2 COS_OEM         /oem
vda3 COS_STATE       /run/initramfs/cos-state
vda4 COS_RECOVERY
vda5 COS_PERSISTENT  /usr/local
vda6 HARV_LH_DEFAULT /var/lib/harvester/defaultdisk

New:

vda
vda1
vda2 COS_OEM         /oem
vda3 COS_RECOVERY
vda4 COS_STATE       /run/initramfs/cos-state
vda5 COS_PERSISTENT  /usr/local
vda6 HARV_LH_DEFAULT /var/lib/harvester/defaultdisk

@tjjh89017
Copy link
Contributor Author

The disk partition order changes as well. Can we stick to the previous one? Old:

vda
vda1
vda2 COS_OEM         /oem
vda3 COS_STATE       /run/initramfs/cos-state
vda4 COS_RECOVERY
vda5 COS_PERSISTENT  /usr/local
vda6 HARV_LH_DEFAULT /var/lib/harvester/defaultdisk

New:

vda
vda1
vda2 COS_OEM         /oem
vda3 COS_RECOVERY
vda4 COS_STATE       /run/initramfs/cos-state
vda5 COS_PERSISTENT  /usr/local
vda6 HARV_LH_DEFAULT /var/lib/harvester/defaultdisk

Elemental cli didn't provide partition order.
It will be elemental limit currently.

@tjjh89017
Copy link
Contributor Author

The "install.poweroff: true" is a bug.
I will dig into it to check the problem.

elemental: bump elemental-cli to v0.1.0
66067f0 
We cannot use Elemental-cli InstallSpec struct to generate config.yaml.
Because there are not all entry with "omitempty", it will cause
generated config.yaml will have several empty entry but didn't be
omitted.
Elemental-cli will treat those empty entry as user-specific value and
override the default value.

Use ElementalInstallSpec to ensure that all entry will be "omitempty".

Signed-off-by: Date Huang <date.huang@suse.com>
@tjjh89017
Copy link
Contributor Author

install.poweroff: true issue is fixed and tested.

@Vicente-Cheng
Copy link
Contributor

Vicente-Cheng commented Dec 15, 2022 •
edited
Loading

The disk partition order changes as well. Can we stick to the previous one? Old:

vda
vda1
vda2 COS_OEM         /oem
vda3 COS_STATE       /run/initramfs/cos-state
vda4 COS_RECOVERY
vda5 COS_PERSISTENT  /usr/local
vda6 HARV_LH_DEFAULT /var/lib/harvester/defaultdisk

New:

vda
vda1
vda2 COS_OEM         /oem
vda3 COS_RECOVERY
vda4 COS_STATE       /run/initramfs/cos-state
vda5 COS_PERSISTENT  /usr/local
vda6 HARV_LH_DEFAULT /var/lib/harvester/defaultdisk

Elemental cli didn't provide partition order. It will be elemental limit currently.

Could we check about the related changes (maybe PRs) on elemental-cli?
I wonder if something depending on the specific partition "number" would meet some problems.

@tjjh89017
Copy link
Contributor Author

The disk partition order changes as well. Can we stick to the previous one? Old:

vda
vda1
vda2 COS_OEM         /oem
vda3 COS_STATE       /run/initramfs/cos-state
vda4 COS_RECOVERY
vda5 COS_PERSISTENT  /usr/local
vda6 HARV_LH_DEFAULT /var/lib/harvester/defaultdisk

New:

vda
vda1
vda2 COS_OEM         /oem
vda3 COS_RECOVERY
vda4 COS_STATE       /run/initramfs/cos-state
vda5 COS_PERSISTENT  /usr/local
vda6 HARV_LH_DEFAULT /var/lib/harvester/defaultdisk

Elemental cli didn't provide partition order. It will be elemental limit currently.

Could we check about the related changes (maybe PRs) on elemental-cli? I wonder if something depending on the specific partition "number" would meet some problems.

https://github.com/rancher/elemental-cli/blob/v0.1.0/pkg/types/v1/config.go#L359-L394

PartitionsByInstallOrder has a fixed order for basic 4 partitions.
So it will be hard limit here if Elemental team will not change those.

Vicente-Cheng
Vicente-Cheng approved these changes Dec 26, 2022
View reviewed changes
Copy link
Contributor

@Vicente-Cheng Vicente-Cheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, test with fresh installation with #373. Thanks!

bk201
bk201 approved these changes Dec 28, 2022
View reviewed changes
Copy link
Member

@bk201 bk201 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@bk201
Copy link
Member

bk201 commented Dec 28, 2022

Please help tag a new os2 release and bump the OS to let CI run again.

@tjjh89017
bump os2 for elemental v0.1.0
4decc63 
Signed-off-by: Date Huang <tjjh89017@hotmail.com>
@bk201 bk201 merged commit 44cfcb1 into harvester:master Dec 28, 2022
@tjjh89017 tjjh89017 mentioned this pull request Dec 28, 2022
Closed
@bk201
Copy link
Member

bk201 commented Jan 3, 2023

@Mergifyio backport v1.1

@mergify mergify bot mentioned this pull request Jan 3, 2023
Merged
@mergify
Copy link

mergify bot commented Jan 3, 2023

backport v1.1

✅ Backports have been created

@masteryyh masteryyh mentioned this pull request Feb 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vicente-Cheng Vicente-Cheng Vicente-Cheng approved these changes

@bk201 bk201 bk201 approved these changes

@starbops starbops starbops approved these changes

@masteryyh masteryyh Awaiting requested review from masteryyh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tjjh89017 @bk201 @Vicente-Cheng @starbops