MULTIARCH-5190: Promote ImageStreamImportMode to default#2266
MULTIARCH-5190: Promote ImageStreamImportMode to default#2266sdodson merged 1 commit intoopenshift:masterfrom
Conversation
|
@Prashanth684: This pull request references MULTIARCH-5190 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Hello @Prashanth684! Some important instructions when contributing to openshift/api: |
|
verify-feature-promotion is failing with: The tests that were added seem to not be running for baremetal - not sure why that's the case. This feature is platform agnostic though so it shouldn't matter. There are 3 tests defined, but only 2 run because the importmode field already has the default |
|
/cc @JoelSpeed is it a hard and fast requirement to have "5" tests ? |
|
Could we see if we can work out why these aren't running on metal? |
Generally the expectation is that it shouldn't be too hard to write 5 tests for a feature. In this case, can you explain the test cases you have, and why they completely cover the feature and additional tests should not be needed? |
The ImportMode default used to be
There are three test cases. All the test cases check the value of
Only two tests run due to the first part of the feature where the default import mode is set based on the CV's architecture in the desired status. |
ah.. i think it's because they are part of the serial test suite and metal only runs the parallel test suite: |
|
/retest |
1 similar comment
|
/retest |
|
/override ci/prow/verify-crd-schema Existing failure that cannot be resolved now |
Metal runs the serial suite for the stable featureset, but doesn't currently for tech preview, this should be fixed so that we can get signal |
|
@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema DetailsIn 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-sigs/prow repository. |
|
Some jobs are failed, when will the pr can be merged? thanks |
|
/test verify-feature-promotion |
|
We need to get more data according to the verify feature promotion job, any idea why one of the tests has such infrequent runs compared to the others? We are looking for at least 14 runs, and a 95% pass rate across the tests |
As discussed, I enabled the one test which was consistently being skipped and added another test to check the arch field in the clusterversion: openshift/origin#29808. The sippy dashboard for the featuregate now shows consistent runs for the tests - the one added recently needs some more runs, but eventually it should also have good signal. There are a few reds i see and these are:
|
this brings the test count to 4 - one short of 5 - but maybe after seeing consistent runs of these tests, we can make an exception? |
|
@JoelSpeed revisiting this again after fixing some failing tests - the board looks green overall for this featuregate. The verify-feature-promotion will still fail as there are only 4 tests associated with the feature. Can we make an exception for this ? |
|
@Prashanth684 are you confident that the 4 tests you have are sufficient for covering the entire feature here? There's no gaps in the testing that you're aware of? Can we get this PR rebased and get a fresh run of the |
49d3888 to
7d6dded
Compare
Yes, the feature is tested in its entirety through the 4 tests. In addition to the three tests mentioned here, the 4th test also checks the CVO's desired.Status.Architecture field to check if it is set appropriately when the release paylaod is multi vs single. |
|
@Prashanth684 Is this feature expected to be supported on SNO? Can we gangway trigger 5 metal runs to boost the stats there and show we are passing with at least 14 runs? |
668769f to
088d7cd
Compare
WalkthroughRemoves Image CRD manifests, adds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (13)
💤 Files with no reviewable changes (8)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
|
/test verify-feature-promotion |
2 similar comments
|
/test verify-feature-promotion |
|
/test verify-feature-promotion |
The ImageStreamImportMode feature gate was introduced in 4.18 and tests were added around the 4.19 timeframe. The feature has been tested for a while now and the sippy dashboard looks healthy. There are 3 tests which were added to exercise the testing of this feature.
088d7cd to
42f4458
Compare
|
@Prashanth684: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
Promotion verification is failing on 4/5 tests but it has been confirmed that 4 tests is sufficient to fully test this feature. @sdodson ACKed that this is OK to merge post "pencils down". /override ci/prow/verify-feature-promotion |
|
/pipeline required |
|
Scheduling tests matching the |
|
/verified by CI |
|
@sdodson: This PR has been marked as verified by DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@everettraven: Overrode contexts on behalf of everettraven: ci/prow/verify-feature-promotion DetailsIn 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-sigs/prow repository. |
The following test was failing for multi clusters: ``` [sig-cluster-lifecycle][OCPFeatureGate:ImageStreamImportMode] ClusterVersion API desired architecture should be valid when architecture is set in release payload metadata [apigroup:config.openshift.io] [Suite:openshift/conformance/parallel] ``` This was because openshift/api#2266 promoted the ImageStreamImportMode featuregate to default, but CVO was not updaing the desired.Architecture status field because it did not exist in the CRD.
|
/payload-aggregate periodic-ci-openshift-release-master-nightly-4.21-e2e-aws-ovn-upgrade-fips 10 |
|
@neisw: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e9f2e510-d42a-11f0-956b-1d9cdb7038d8-0 |
The ImageStreamImportMode feature gate was introduced in 4.18 and tests were added around the 4.19 timeframe. The feature has been tested for a while now and the sippy dashboard looks healthy. There are 3 tests that were added to exercise the testing of this feature.