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

Bug 1691660: pkg/controller: always use the OSImageURL from the CVO #475

Merged
merged 1 commit into from
Mar 25, 2019
Merged

Bug 1691660: pkg/controller: always use the OSImageURL from the CVO #475

merged 1 commit into from
Mar 25, 2019

Conversation

runcom
Copy link
Member

@runcom runcom commented Feb 22, 2019 •
edited

Signed-off-by: Antonio Murdaca runcom@linux.com

- What I did

Always use the osimageurl coming from the CVO.

close #465

- How to verify it

Added a unit test to cover this change

- Description for the changelog

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 22, 2019
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 22, 2019
@runcom
Copy link
Member Author

runcom commented Feb 22, 2019

/retest

@kikisdeliveryservice
Copy link
Contributor

Taking a look at this, there've been a couple PRs that change the render/controller config functions so refamiliarizing myself.

@cgwalters
Copy link
Member

/approve
I like the idea though am uncertain about landing this right now given the testing we've done with the existing implementation. But it looks generally good to me.

@runcom
Copy link
Member Author

runcom commented Feb 22, 2019

I'm uncertain about landing this as well, but w/o this (assuming it works and I can add e2e) customers and users will be able to upgrade/downgrade the baseos through MCs which id love to avoid

@kikisdeliveryservice
Copy link
Contributor

q: are we sure we don't want to allow this at all? just wondering before we go to the trouble of landing this and then having it reversed..

that said, i think this should be held until later when the os upgrades are healthy generally.

@runcom
Copy link
Member Author

runcom commented Feb 22, 2019 •
edited

q: are we sure we don't want to allow this at all?

I don't see any real value in allowing them doing something like that, we want to control upgrades (generally) through CVO, i've always seen osimageurl in the MCs like something for testing for now, but if you all see a value in allowing users doing this, yeah, let's defer (though, do we provide them with oscontainers? will they have access to the registry containing those?

@runcom
Copy link
Member Author

runcom commented Feb 22, 2019

This has become more relevant in some hours as upgrades should follow the pattern where master are always upgraded before workers...we can play and check what users pass us of course and compare between pools but I feel the cleanest way to do it is to just forbid osimageurl altogether

@runcom
Copy link
Member Author

runcom commented Feb 23, 2019

question now is, since #480, do we want to go out with the possibility for users to do this #183 (comment) and then break them?

LorbusChris
LorbusChris reviewed Feb 25, 2019
View reviewed changes
pkg/apis/machineconfiguration.openshift.io/v1/helpers.go Outdated
outOSImageURL = config.Spec.OSImageURL
mc := configs[idx]
if mc.Spec.OSImageURL != osImageURL {
glog.Infof("Detected OSImageURL %q in MachineConfig %q which is not what provided by the CVO %q", mc.Spec.OSImageURL, mc.Name, osImageURL)
Copy link
Member

Choose a reason for hiding this comment

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

nit: what is provided

@runcom runcom changed the title [WIP] pkg/controller: always use the OSImageURL from the CVO pkg/controller: always use the OSImageURL from the CVO Feb 25, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2019
@runcom
Copy link
Member Author

runcom commented Feb 25, 2019

removing WIP here, up for further discussion as I do really feel we shouldn't really expose this

@runcom runcom force-pushed the no-override-osimageurl branch 2 times, most recently from 2e6efb2 to d6be234 Compare February 25, 2019 22:17
@runcom
Copy link
Member Author

runcom commented Feb 25, 2019

The development flow that this might break could be handled through #496 overriding machine-os-content directly (even if you can't really control the pool, but hey, we want #480 anyway)

@cgwalters
Copy link
Member

I think my vote on this is to land tests for OS updates first. The code here looks reasonable but it's also a nontrivial change; we know today that oc adm upgrade works. The problem is that doing testing here nicely depends on #496

@cgwalters
Copy link
Member

removing WIP here, up for further discussion as I do really feel we shouldn't really expose this

I understand that but on the other hand...an admin really has to jump through hoops today to actually override osImageURL.

@runcom
Copy link
Member Author

runcom commented Feb 27, 2019

I think my vote on this is to land tests for OS updates first.

I'm super ok with this of course

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2019
@cgwalters
Copy link
Member

One thing I think we could envision now potentially is adding a new custommachineconfig CRD or so that is like machineconfig but for users...and wouldn't have osImageURL.

Or alternatively, move our rendered ones into a new renderedmachineconfig CRD?

@runcom
Copy link
Member Author

runcom commented Mar 20, 2019

The second option sounds very reasonable to me :) (having a rendered MC CRD)

@runcom
Copy link
Member Author

runcom commented Mar 23, 2019 •
edited

Related to https://bugzilla.redhat.com/show_bug.cgi?id=1691660 now.
Not sure how many users are now using osimageurl directly in machineconfigs, now that upgrades are generally working fine.
The related Bugzilla just highlights a flaw where if we have an user provided MC with osimageurl, then we don't respect the machineoscontent from the payload, which is wrong. I don't think there's a nice way to still honor the payload while leaving an osimageurl MC around so probably the time to just get rid of it.

This PR should land to also prevent QE test upgrades through osImageURL which is: unexpected and unsupported IMO. QE should test by creating a payload overriding machine-os-content and that should be it. Testing with osImageURL isn't real testing, just excercising a code that is excercised correctly through a normal upgrade test.

I'd also add that it would be safer to go out w/o allowing osimageurl and then, if we change design or our mind allowing it.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 23, 2019
@runcom runcom force-pushed the no-override-osimageurl branch 2 times, most recently from 8ab36f5 to 9558238 Compare March 23, 2019 19:13
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2019
@runcom
Copy link
Member Author

runcom commented Mar 23, 2019

This is rebased now and ready to go!

@runcom
Copy link
Member Author

runcom commented Mar 23, 2019

/retest

@runcom runcom changed the title pkg/controller: always use the OSImageURL from the CVO Bug 1691660: pkg/controller: always use the OSImageURL from the CVO Mar 23, 2019
@runcom
Copy link
Member Author

runcom commented Mar 24, 2019

/retest

@runcom
pkg/controller: always use the OSImageURL from the CVO
a5af821 
Signed-off-by: Antonio Murdaca <runcom@linux.com>
@runcom
Copy link
Member Author

runcom commented Mar 24, 2019

/retest

1 similar comment
@runcom
Copy link
Member Author

runcom commented Mar 25, 2019

/retest

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, runcom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 31f4139 into openshift:master Mar 25, 2019
@runcom runcom deleted the no-override-osimageurl branch March 25, 2019 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgwalters cgwalters cgwalters left review comments

@LorbusChris LorbusChris LorbusChris left review comments

@ashcrow ashcrow Awaiting requested review from ashcrow

@wking wking Awaiting requested review from wking

Assignees

@cgwalters cgwalters

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prevent OSImageURL from being set by users
6 participants
@runcom @kikisdeliveryservice @cgwalters @openshift-ci-robot @LorbusChris @openshift-merge-robot