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.

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

Manage user data #2827

Merged
merged 4 commits into from
Dec 8, 2021
Merged

Conversation

zaneb
Copy link
Member

@zaneb zaneb commented Nov 9, 2021

@zaneb
Copy link
Member Author

zaneb commented Nov 9, 2021

/cc @hardys @cgwalters

zaneb and others added 2 commits November 9, 2021 17:28
This makes it easier to reuse parts of the function.
With this we let the installer take care of installing the initial
user-data secret and we then take over with our managed secret. if we
are upgrading (and thus no installer at play), we just create the new
(managed) secret.

This is a cherry-pick of the original patch
1f52e48, which was later reverted by
1c65355.

Signed-off-by: Antonio Murdaca <runcom@linux.com>
Co-authored-by: Zane Bitter <zbitter@redhat.com>
@zaneb
Copy link
Member Author

zaneb commented Nov 10, 2021

/retest-required

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

OK I re-skimmed the previous discussion. I have to admit I can just like barely keep all this in my head and it will surely get context switched out again. But your commit message was great in linking to all of the relevant bits.

So...as far as I understand things, the next required step here would be for the installer to switch over to generating machinesets to point to -user-data-managed if it exists?

Marking approved from my PoV but this also requires an MCO team approval I'd say.

pkg/controller/common/helpers.go Outdated Show resolved Hide resolved
yuqi-zhang and others added 2 commits November 10, 2021 16:08
This will allow us to manage a secret to scale up nodes with
ignition v2 binary installed.

Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
(cherry picked from commit 22b8b24)
@zaneb
Copy link
Member Author

zaneb commented Nov 10, 2021

So...as far as I understand things, the next required step here would be for the installer to switch over to generating machinesets to point to -user-data-managed if it exists?

Yeah, I will need to follow up by also unreverting openshift/installer#4228 and openshift/hive#1166.

@cgwalters
Copy link
Member

cgwalters commented Nov 10, 2021

Yeah, I will need to follow up by also unreverting openshift/installer#4228 and openshift/hive#1166.

Oh man, I hadn't realized hive needed changes for this too. It'd be really nice to somehow have that share code with openshift/installer.

And just for completeness, now that https://github.com/openshift/hypershift exists does it need updating too? A quick code search turns up openshift/hypershift@d98c7a7 which seems to have dropped that dependency?

@zaneb
Copy link
Member Author

zaneb commented Nov 11, 2021

Oh man, I hadn't realized hive needed changes for this too. It'd be really nice to somehow have that share code with openshift/installer.

Me neither, I just found it looking at that installer PR.
I don't think it can share code because it needs to do different things based on the version of the cluster being installed :(

And just for completeness, now that https://github.com/openshift/hypershift exists does it need updating too?

As far as I can tell, no. I don't see any reference to [master|worker]-user-data in that repo.

@zaneb
Copy link
Member Author

zaneb commented Nov 12, 2021

/retest-required

@zaneb
Copy link
Member Author

zaneb commented Nov 12, 2021

OK, I think it was actually a mistake for the installer change to modify the MachineSets for every platform - to date all of those MachineSets have fixed images that never change, so their user data should never change. The only platform we should switch over the worker image names for is baremetal.

The baremetal platform doesn't use MachinePools in Hive, so if we limit the change to that then there is nothing to do in Hive.

zaneb added a commit to zaneb/openshift-installer that referenced this pull request Nov 12, 2021
Since openshift/machine-config-operator#2827, the MCO creates managed
master and worker ignition stub configs to ensure they always use the
latest version of the ignition format.

Create new master Machines using the master-user-data-managed Secret,
and install the initial user data in this Secret instead of the old
master-user-data Secret.

This is the part of the installer changes for the enhancement:
https://github.com/openshift/enhancements/blob/master/enhancements/machine-config/user-data-secret-managed.md

A previous version of this patch (but for both masters and workers) was
previously committed as 8d278d2, but
later reverted by 3920ae4.
@zaneb
Copy link
Member Author

zaneb commented Nov 16, 2021

/retest

5 similar comments
@zaneb
Copy link
Member Author

zaneb commented Nov 22, 2021

/retest

@zaneb
Copy link
Member Author

zaneb commented Nov 22, 2021

/retest

@zaneb
Copy link
Member Author

zaneb commented Nov 23, 2021

/retest

@zaneb
Copy link
Member Author

zaneb commented Nov 24, 2021

/retest

@zaneb
Copy link
Member Author

zaneb commented Nov 30, 2021

/retest

@hardys
Copy link
Contributor

hardys commented Dec 1, 2021

/lgtm

So just to clarify - the reason this got reverted is it broke the installer interface that allows users to customize the pointer ignition.

As mentioned in the description I subsequently made changes to the installer which ensure customizations to the pointer config are translated to a MachineConfig, so it should be safe to restore this AFAICS.

/cc @yuqi-zhang @runcom

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2021
@zaneb
Copy link
Member Author

zaneb commented Dec 1, 2021

It appears to me that the remaining tests here are perma-failing for reasons unrelated to this patch.

/assign @sinnykumari
/retest

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@zaneb
Copy link
Member Author

zaneb commented Dec 7, 2021

Is it worth overriding the e2e-agnostic-upgrade job, which seems to be broken eveywhere for everyone?

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

11 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 8, 2021

@zaneb: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/okd-e2e-aws 59f1381 link false /test okd-e2e-aws
ci/prow/e2e-aws-workers-rhel7 59f1381 link false /test e2e-aws-workers-rhel7
ci/prow/e2e-aws-disruptive 59f1381 link false /test e2e-aws-disruptive

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit b50080b into openshift:master Dec 8, 2021
zaneb added a commit to zaneb/openshift-installer that referenced this pull request Dec 16, 2021
Since openshift/machine-config-operator#2827, the MCO creates managed
master and worker ignition stub configs to ensure they always use the
latest version of the ignition format.

While currently we don't have an automated way of recreating master
nodes, on the baremetal platform we now ship an up-to-date image as part
of the release payload, and it would be expected that any reprovisioning
of Machines would use this image.

Therefore, on the baremetal platform, create new master Machines using
the master-user-data-managed Secret, and install the initial user data
in this Secret instead of the old master-user-data Secret.

This is the part of the installer changes for the enhancement:
https://github.com/openshift/enhancements/blob/master/enhancements/machine-config/user-data-secret-managed.md

A previous version of this patch (but for both masters and workers, on
all platforms) was previously committed in
8d278d2, but later reverted by
3920ae4.
zaneb added a commit to zaneb/openshift-installer that referenced this pull request Dec 16, 2021
Since openshift/machine-config-operator#2827, the MCO creates managed
master and worker ignition stub configs to ensure they always use the
latest version of the ignition format.

While currently we don't have an automated way of recreating master
nodes, on the baremetal platform we now ship an up-to-date image as part
of the release payload, and it would be expected that any reprovisioning
of Machines would use this image.

Therefore, on the baremetal platform, create new master Machines using
the master-user-data-managed Secret, and install the initial user data
in this Secret instead of the old master-user-data Secret.

This is the part of the installer changes for the enhancement:
https://github.com/openshift/enhancements/blob/master/enhancements/machine-config/user-data-secret-managed.md

A previous version of this patch (but for both masters and workers, on
all platforms) was previously committed in
8d278d2, but later reverted by
3920ae4.
zaneb added a commit to zaneb/openshift-installer that referenced this pull request Dec 16, 2021
Since openshift/machine-config-operator#2827, the MCO creates managed
master and worker ignition stub configs to ensure they always use the
latest version of the ignition format.

While currently we don't have an automated way of recreating master
nodes, on the baremetal platform we now ship an up-to-date image as part
of the release payload, and it would be expected that any reprovisioning
of Machines would use this image.

Therefore, on the baremetal platform, create new master Machines using
the master-user-data-managed Secret, and install the initial user data
in this Secret instead of the old master-user-data Secret.

This is the part of the installer changes for the enhancement:
https://github.com/openshift/enhancements/blob/master/enhancements/machine-config/user-data-secret-managed.md

A previous version of this patch (but for both masters and workers, on
all platforms) was previously committed in
8d278d2, but later reverted by
3920ae4.
zaneb added a commit to zaneb/openshift-installer that referenced this pull request Dec 16, 2021
Since openshift/machine-config-operator#2827,
the MCO creates managed master and worker ignition stub configs to
ensure they always use the latest version of the ignition format.

While currently we don't have an automated way of recreating master
nodes, on the baremetal platform we now ship an up-to-date image as part
of the release payload, and it would be expected that any reprovisioning
of Machines would use this image.

Therefore, on the baremetal platform, create new master Machines using
the master-user-data-managed Secret, and install the initial user data
in this Secret instead of the old master-user-data Secret.

This is the part of the installer changes for the enhancement:
https://github.com/openshift/enhancements/blob/master/enhancements/machine-config/user-data-secret-managed.md

A previous version of this patch (but for both masters and workers, on
all platforms) was previously committed in
8d278d2, but later reverted by
3920ae4.
zaneb added a commit to zaneb/openshift-installer that referenced this pull request Dec 16, 2021
On the baremetal platform, worker MachineSets on newly-installed
clusters will automatically use the latest version of CoreOS to
provision, as described in
https://github.com/openshift/enhancements/blob/master/enhancements/baremetal/coreos-image-in-release.md

To ensure that the MachineConfig Ignition stub always uses a compatible
version of the Ignition format, point these MachineSets at the Secret
managed by the MCO (worker-user-data-managed), which is created since
openshift/machine-config-operator#2827. Since a
non-managed version of the Secret is not required on this platform,
install the worker-user-data-managed Secret initially rather than the
previous worker-user-data Secret.

Other platforms will continue to install worker-user-data Secrets,
which are preserved as they were at cluster creation time, because they
are referenced by MachineSets that have fixed CoreOS images. When these
MachineSets are updated to always use the latest CoreOS images, they
should also be updated to point to the managed user data.

This is the part of the installer changes for the enhancement:
https://github.com/openshift/enhancements/blob/master/enhancements/machine-config/user-data-secret-managed.md

A previous version of this patch (but for all platforms) was previously
committed as 8d278d2, but later
reverted by 3920ae4.
@zaneb
Copy link
Member Author

zaneb commented Dec 16, 2021

Installer changes for this are in openshift/installer#5473 (specifically, the last two commits), and affect only the baremetal platform.

zaneb added a commit to zaneb/openshift-installer that referenced this pull request Dec 16, 2021
Since openshift/machine-config-operator#2827,
the MCO creates managed master and worker ignition stub configs to
ensure they always use the latest version of the ignition format.

While currently we don't have an automated way of recreating master
nodes, on the baremetal platform we now ship an up-to-date image as part
of the release payload, and it would be expected that any reprovisioning
of Machines would use this image.

Therefore, on the baremetal platform, create new master Machines using
the master-user-data-managed Secret, and install the initial user data
in this Secret instead of the old master-user-data Secret.

This is the part of the installer changes for the enhancement:
https://github.com/openshift/enhancements/blob/master/enhancements/machine-config/user-data-secret-managed.md

A previous version of this patch (but for both masters and workers, on
all platforms) was previously committed in
8d278d2, but later reverted by
3920ae4.
zaneb added a commit to zaneb/openshift-installer that referenced this pull request Dec 16, 2021
On the baremetal platform, worker MachineSets on newly-installed
clusters will automatically use the latest version of CoreOS to
provision, as described in
https://github.com/openshift/enhancements/blob/master/enhancements/baremetal/coreos-image-in-release.md

To ensure that the MachineConfig Ignition stub always uses a compatible
version of the Ignition format, point these MachineSets at the Secret
managed by the MCO (worker-user-data-managed), which is created since
openshift/machine-config-operator#2827. Since a
non-managed version of the Secret is not required on this platform,
install the worker-user-data-managed Secret initially rather than the
previous worker-user-data Secret.

Other platforms will continue to install worker-user-data Secrets,
which are preserved as they were at cluster creation time, because they
are referenced by MachineSets that have fixed CoreOS images. When these
MachineSets are updated to always use the latest CoreOS images, they
should also be updated to point to the managed user data.

This is the part of the installer changes for the enhancement:
https://github.com/openshift/enhancements/blob/master/enhancements/machine-config/user-data-secret-managed.md

A previous version of this patch (but for all platforms) was previously
committed as 8d278d2, but later
reverted by 3920ae4.
AnnaZivkovic pushed a commit to AnnaZivkovic/installer that referenced this pull request Apr 1, 2022
Since openshift/machine-config-operator#2827,
the MCO creates managed master and worker ignition stub configs to
ensure they always use the latest version of the ignition format.

While currently we don't have an automated way of recreating master
nodes, on the baremetal platform we now ship an up-to-date image as part
of the release payload, and it would be expected that any reprovisioning
of Machines would use this image.

Therefore, on the baremetal platform, create new master Machines using
the master-user-data-managed Secret, and install the initial user data
in this Secret instead of the old master-user-data Secret.

This is the part of the installer changes for the enhancement:
https://github.com/openshift/enhancements/blob/master/enhancements/machine-config/user-data-secret-managed.md

A previous version of this patch (but for both masters and workers, on
all platforms) was previously committed in
8d278d2, but later reverted by
3920ae4.
AnnaZivkovic pushed a commit to AnnaZivkovic/installer that referenced this pull request Apr 1, 2022
On the baremetal platform, worker MachineSets on newly-installed
clusters will automatically use the latest version of CoreOS to
provision, as described in
https://github.com/openshift/enhancements/blob/master/enhancements/baremetal/coreos-image-in-release.md

To ensure that the MachineConfig Ignition stub always uses a compatible
version of the Ignition format, point these MachineSets at the Secret
managed by the MCO (worker-user-data-managed), which is created since
openshift/machine-config-operator#2827. Since a
non-managed version of the Secret is not required on this platform,
install the worker-user-data-managed Secret initially rather than the
previous worker-user-data Secret.

Other platforms will continue to install worker-user-data Secrets,
which are preserved as they were at cluster creation time, because they
are referenced by MachineSets that have fixed CoreOS images. When these
MachineSets are updated to always use the latest CoreOS images, they
should also be updated to point to the managed user data.

This is the part of the installer changes for the enhancement:
https://github.com/openshift/enhancements/blob/master/enhancements/machine-config/user-data-secret-managed.md

A previous version of this patch (but for all platforms) was previously
committed as 8d278d2, but later
reverted by 3920ae4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants