MCO MCO-424: daemon: Remove old legacy OS update path #3583
Conversation
openshift-ci
bot
added
the
approved
cgwalters
force-pushed
the
no-legacy-osupdate
branch
from
11b173e to
c9d8953
Compare
|
Yes,I would love to get rid of this. Holding for pre-merge QE. |
openshift-ci
bot
added
the
do-not-merge/hold
|
/lgtm |
|
I agree that we will most likely won't be using this code path. Although, this is a removal of old approach, so seems like ideal candidate for getting qe approval label as per pre-merge testing. When we are ready with dropping machine-os-content, we can get #3364 and this PR together as part of https://issues.redhat.com/browse/MCO-424. |
cgwalters
force-pushed
the
no-legacy-osupdate
branch
from
c9d8953 to
ed75611
Compare
|
Rebased 🏄
Sure, that's fine. That said, I think a payload job upgrading from 4.13 combined with the presubmits should give pretty good coverage here. /payload-job periodic-ci-openshift-release-master-ci-4.14-upgrade-from-stable-4.13-e2e-gcp-ovn-upgrade I'm however less certain I'd say we should combine this PR and dropping |
|
@cgwalters: trigger 1 job(s) for the /payload-(job|aggregate) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f77e5c20-00b8-11ee-9f26-79e16bfc5388-0 |
cgwalters
force-pushed
the
no-legacy-osupdate
branch
from
ed75611 to
5731660
Compare
cgwalters
force-pushed
the
no-legacy-osupdate
branch
from
5731660 to
628040c
Compare
|
/payload-job periodic-ci-openshift-release-master-ci-4.14-upgrade-from-stable-4.13-e2e-gcp-ovn-upgrade |
|
@cgwalters: trigger 1 job(s) for the /payload-(job|aggregate) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ea117260-0fa0-11ee-886c-2ae9a5b07692-0 |
cgwalters
force-pushed
the
no-legacy-osupdate
branch
from
628040c to
37682d3
Compare
sinnykumari
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@cgwalters: This pull request references MCO-424 which is a valid jira issue. In response to this:
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. |
|
LGTM, thanks for working on this! /assign @sergiordlr for pre-merge testing |
|
Verification using IPI on AWS
The installation finished OK.
The installation was OK.
The negative test cases (those using a non-bootable image and a non-existing image) fail because the error messages have changed. Messages are OK, they are descriptive enough, but we need to adapt the test cases to use the new messages.
No problem has been found up to now. We will wait until the automated cases are finished, if all the test cases pass we will add the qe-approved label. I'm sorry, but the tests take a big amount of time to be executed, so probably we will have to add the label on Monday. |
|
Verification using IPI on AWS. Apart from the steps described in the previous comment. We executed:
No problems found.
42365-add real time kernel argument No problems found. (Negative cases need to be adapted, though). We can add the qe-approved label /label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
/retest-required Remaining retests: 0 against base HEAD 6d8204f and 2 for PR HEAD 37682d37513a327ad07663c608e0d4119bf7fd06 in total |
|
/test e2e-gcp-op |
|
/retest-required Remaining retests: 0 against base HEAD 85fe0da and 1 for PR HEAD 37682d37513a327ad07663c608e0d4119bf7fd06 in total |
|
/retest-required Remaining retests: 0 against base HEAD 5ddeae5 and 0 for PR HEAD 37682d37513a327ad07663c608e0d4119bf7fd06 in total |
|
Ah recent merge lead to merge conflict for this PR. Needs rebasing. |
openshift-merge-robot
added
the
needs-rebase
This must be dead code in 4.14. There's no going back. Everything in 4.12 really should have transitioned, and certainly 4.13 and very much most definitely 4.14. Deleting code is good on general principle, but very specifically this cost me hours of debugging because I modified the legacy update path when trying something, not the new one.
cgwalters
force-pushed
the
no-legacy-osupdate
branch
from
37682d3 to
85b297c
Compare
openshift-merge-robot
removed
the
needs-rebase
|
OK yep, this PR to remove completely dead code is now on at least its 3rd rebase due to conflicts - i.e. people were spending time changing completely unused code. |
|
hopefully the general CI issues will resolve and we can get this in without a 4th rebase 🙂 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, jkyros, sinnykumari 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 |
|
/retest-required |
|
/hold Revision 85b297c was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
|
@cgwalters: The following tests failed, say
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. |
|
Looks like a flake /test e2e-aws-ovn |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
This must be dead code in 4.14. There's no going back. Everything in 4.12 really should have transitioned, and certainly 4.13 and very much most definitely 4.14.
Deleting code is good on general principle, but very specifically this cost me hours of debugging because I modified the legacy update path when trying something, not the new one.