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

USHIFT-704: skip [sig-cli] whoami result with console for MicroShift #28005

Merged

Conversation

chiragkyal
Copy link
Member

@chiragkyal chiragkyal commented Jun 27, 2023

This change will skip
"[sig-cli] oc basics can show correct whoami result with console [Skipped:NoOptionalCapabilities] [Suite:openshift/conformance/parallel]"
test by adding NoOptionalCapabilities for MicroShift

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 27, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jun 27, 2023

@chiragkyal: This pull request references USHIFT-704 which is a valid jira issue.

In response to this:

This change will skip
"[sig-cli] oc basics can show correct whoami result with console [Skipped:NoOptionalCapabilities] [Suite:openshift/conformance/parallel]"
test if openshift-config-managed namespace is not present. This namespace holds console-public configmap having consoleURL.

For MicroShiftopenshift-config-managed namespace is not present, so this test can be skipped.

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.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 27, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 27, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@@ -250,6 +250,11 @@ var _ = g.Describe("[sig-cli] oc basics", func() {
})

g.It("can show correct whoami result with console", func() {
nsExist, err := exutil.IsNamespaceExist(oc, "openshift-config-managed")
Copy link
Contributor

Choose a reason for hiding this comment

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

How is that namespace related to this test? Is that where the console runs? Or does the whoami command use information from that namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

whoami command use information from that namespace.

It looks for console-public configmap present inside that namespace, to extract consoleURL. It's defined here and here. We don't have this namespace in MicroShift.

Copy link
Contributor

Choose a reason for hiding this comment

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

The console is an optional component now. What does whoami do if the namespace exists but the console URL is empty? Maybe there's a bug in whoami for this more extreme case where the console URL cannot be determined?

Copy link
Contributor

Choose a reason for hiding this comment

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

it only is an issue if the "--show-console" arg is specified. If you specify that arg when you don't have the console enabled, you'll get an error but that should not be surprising.

https://github.com/openshift/oc/blob/c078876cc5aa434a02f14c28b8c2a56b9011aa13/pkg/cli/whoami/whoami.go#L149-L150

https://github.com/openshift/oc/blob/c078876cc5aa434a02f14c28b8c2a56b9011aa13/pkg/cli/whoami/whoami.go#L121-L136

i feel like we had to deal w/ this for the "nocaps" e2e job, but since this test doesn't seem to contain any logic to check for which caps are enabled, i'm not sure how we ended up doing that.

Copy link
Contributor

Choose a reason for hiding this comment

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

118d288 adds this test to a set annotated with [Skipped:NoOptionalCapabilities]. In test/extended/util/annotate/rules.go that maps to requires-optional-cap, which is left out of no optional capabilities in the test case list defined in TestDecodeProvider. That entry has platform set to GCE, though, and MicroShift runs the tests with provider none.

Can we do something else in the setup before running the tests for MicroShift to indicate that all capabilities are disabled and that no optional capabilities configuration should be used? Do we need another entry in the list in TestDecodeProvider for MicroShift?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need another entry in the list in TestDecodeProvider for MicroShift?

No, that's just a test case for ensuring we detect/decode clusters correctly (so that we can pick the right set of tests). I think you need to update the cluster discovery logic to recognize a "microshift" cluster and set the EnabledCapabilities accordingly:

func decodeProvider(provider string, dryRun, discover bool, clusterState *exutilcluster.ClusterState) (*exutilcluster.ClusterConfiguration, error) {

clusterVersion, err := configClient.ConfigV1().ClusterVersions().Get(context.Background(), "version", metav1.GetOptions{})
if err != nil {
return nil, err
}
state.OptionalCapabilities = clusterVersion.Status.Capabilities.EnabledCapabilities

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the decodeProvider logic to recognize a "microshift" cluster and set config.HasNoOptionalCapabilities = true. However, the [Skipped:NoOptionalCapabilities] annotation doesn't seem to be getting enabled for this test. Any suggestions that I may be missing here?

@chiragkyal chiragkyal marked this pull request as ready for review June 28, 2023 10:30
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2023
@openshift-ci openshift-ci bot requested review from bparees and mfojtik June 28, 2023 10:32
@openshift-merge-robot openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 29, 2023
@chiragkyal
Copy link
Member Author

/retest

}
if isMicroShift {
config.HasNoOptionalCapabilities = true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure you're ending on up in the "Default" provider case when running against a microshift cluster?

should be easy enough to run locally w/ some debug statements and see what's actually happening when the detection logic runs

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it should be inside "none" case. There was something wrong with my MicroShift cluster which leads all debug statements to go inside "Default" case.

I've added it now in "none" case.

if err != nil {
return nil, err
}
isMicroShift, err := exutil.IsMicroShiftCluster(coreClient)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would do this in "DiscoverClusterState" and not "decodeProvider".

The provider is irrelevant

Copy link
Member Author

Choose a reason for hiding this comment

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

Most of the cluster state logic checks inside "DiscoverClusterState" are not applicable for MicroShift. For example:

infrastructures.config.openshift.io
networks.operator.openshift.io
clusterversion

If we conditionally try to skip the above checks based on IsMicroShiftCluster() in "DiscoverClusterState", then I think, we also need to update "LoadConfig()", which would be called after "DiscoverClusterState".

/cc @pacevedom

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah ok, since we don't call DiscoverClusterState in the case of provider==none, it's not a good fit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with Ben, even though the name might be misleading because its more discovery than decoding.

@bparees
Copy link
Contributor

bparees commented Jul 10, 2023

lgtm, is there a microshift job we can run this PR against?

@chiragkyal
Copy link
Member Author

lgtm, is there a microshift job we can run this PR against?

Not yet. Currently, one by one we are fixing the tests which are failing in MicroShift. Ultimately, our target is to have a working job, passing all the tests.

@bparees
Copy link
Contributor

bparees commented Jul 10, 2023

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2023
@dhellmann
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 10, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 10, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, chiragkyal, dhellmann

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-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 98e2f3f and 2 for PR HEAD 4a72ef7 in total

@chiragkyal
Copy link
Member Author

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD a60975f and 1 for PR HEAD 4a72ef7 in total

@chiragkyal
Copy link
Member Author

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 158f668 and 0 for PR HEAD 4a72ef7 in total

@openshift-ci-robot
Copy link

/hold

Revision 4a72ef7 was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2023
@chiragkyal
Copy link
Member Author

/unhold
/retest

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2023
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 158f668 and 2 for PR HEAD 4a72ef7 in total

@chiragkyal
Copy link
Member Author

/retest-required

1 similar comment
@chiragkyal
Copy link
Member Author

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD a02345e and 1 for PR HEAD 4a72ef7 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD f352e7c and 0 for PR HEAD 4a72ef7 in total

@openshift-ci-robot
Copy link

/hold

Revision 4a72ef7 was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2023
@chiragkyal
Copy link
Member Author

/unhold
/retest-required

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2023
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 9609ce2 and 2 for PR HEAD 4a72ef7 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD f907888 and 1 for PR HEAD 4a72ef7 in total

@chiragkyal
Copy link
Member Author

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD c945bb9 and 0 for PR HEAD 4a72ef7 in total

@openshift-ci-robot
Copy link

/hold

Revision 4a72ef7 was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2023
@chiragkyal
Copy link
Member Author

/unhold
/retest-required

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2023
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD c945bb9 and 2 for PR HEAD 4a72ef7 in total

@chiragkyal
Copy link
Member Author

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 18, 2023

@chiragkyal: all tests passed!

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-merge-robot openshift-merge-robot merged commit 7ed0335 into openshift:master Jul 18, 2023
23 checks passed
@chiragkyal chiragkyal deleted the USHIFT-704-console branch July 18, 2023 17:28
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. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants