MCO-1911: Migrate the mco_password test suite from the private repository to the MCO repository #5374
Conversation
|
@sergiordlr: This pull request references MCO-1911 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.21.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
sergiordlr
force-pushed
the
MCO-1911-migrate-password-tests
branch
from
9be0560 to
d0fee92
Compare
|
@sergiordlr: This pull request references MCO-1911 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.21.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@sergiordlr: This pull request references MCO-1911 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.21.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/payload-job periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-gcp-mco-disruptive-techpreview |
|
@isabella-janssen: 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/3d3d5610-b108-11f0-98c7-8ba6413e2fde-0 |
sergiordlr
force-pushed
the
MCO-1911-migrate-password-tests
branch
3 times, most recently
from
f7af459 to
c0b5ea5
Compare
|
/payload-job periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-gcp-mco-disruptive-techpreview |
|
@sergiordlr: 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/65739b80-b356-11f0-92e2-878f29f03021-0 |
|
/test ci/prow/bootstrap-unit |
|
@sergiordlr: The specified target(s) for The following commands are available to trigger optional jobs: Use 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-sigs/prow repository. |
|
/test bootstrap-unit |
|
/test bootstrap-unit |
test/extended/events.go
Outdated
| } | ||
|
|
||
| // GetAllSince return a list of the events that happened since the provided duration | ||
| func (el EventList) GetAllSince(since time.Time) ([]Event, error) { |
There was a problem hiding this comment.
There's a mix of receiver declarations. Some receivers, even for the same type use pointers others vaues. For a given type, all receivers should be declared the same. I usually lean towards pointer receivers unless the type is a dumb read-only struct.
test/extended/controller.go
Outdated
| } | ||
|
|
||
| // GetIgnoredLogs returns the logs that will be ignored after calling "IgnoreLogsBeforeNow" | ||
| func (mcc Controller) GetIgnoredLogs() string { |
test/extended/node.go
Outdated
| } | ||
|
|
||
| // IsReady returns if the node is in Ready condition | ||
| func (n Node) IsReady() bool { |
test/extended/remotefile.go
Outdated
| } | ||
|
|
||
| // GetFilteredTextContent returns the filetered remote file's text content as a list of strings, one string per line matching the regexp. | ||
| func (rf RemoteFile) GetFilteredTextContent(regex string) ([]string, error) { |
test/extended/remotefile.go
Outdated
| } | ||
|
|
||
| for i, name := range re.SubexpNames() { | ||
| if i < 0 { |
There was a problem hiding this comment.
The index of an slice iterator cannot be less than zero. This error condition cannot happen.
test/extended/events.go
Outdated
| for _, event := range events { | ||
| output += fmt.Sprintf("- %s\n", event) | ||
| } | ||
| output += output + fmt.Sprintf("NOT to contain this reason sequence\n\t%s\n", matcher.sequence) |
There was a problem hiding this comment.
A catch from Claude Code: This line duplicating the output content, remove the output +
There was a problem hiding this comment.
I see some loop/string manipulations with the event list. I'd advocate to improve its handling a bit, cause the list of events in a run is huge and performance in terms of speed and memory needs to be taken into consideration.
For string manipulation inside loops, give the strings.Builder a look.
There was a problem hiding this comment.
We use strings.Builder to create the log strings now
test/extended/events.go
Outdated
|
|
||
| // To avoid too many "oc" executions we store the events information in a cached struct list with "lastTimestamp" and "reason" fields. | ||
| tmpEvents := []tmpEvent{} | ||
| for _, loopEvent := range events { |
There was a problem hiding this comment.
Given that the events slice may be big and its size is known at this point I suggest to pre-allocate the tmpEvents slice.
| event.oc.NotShowInfo() | ||
| defer event.oc.SetShowInfo() | ||
|
|
||
| lastOccurrence, err := event.GetLastTimestamp() |
There was a problem hiding this comment.
Ouch, so, we fetch all the events, that again, can be a huge pile, and we make N calls to oc just to get their timestamp. That's far from good. Couldn't we just tell GetAll to fetch the complete definition of the events and store it for later usages, like getting the timestamp?
There was a problem hiding this comment.
I would like to add this logic to the Resource struct, so that it can be used by all resources and not only by the events.
Would you mind if I implement this change once we finish the migration, please?
GetAll is not fetching any data with the current code, so we need to execute a command to fetch the timestamp anyway in this scenario.
test/extended/machineconfig.go
Outdated
| } | ||
|
|
||
| // delete deletes the MachineConfig and waits for the MCP to be updated | ||
| func (mc *MachineConfig) delete() { |
There was a problem hiding this comment.
Why is this one private?
There was a problem hiding this comment.
We have renamed the method to "DeleteAndWait", matching its current actual behaviour.
sergiordlr
force-pushed
the
MCO-1911-migrate-password-tests
branch
from
c0b5ea5 to
4c384c8
Compare
test/extended/events.go
Outdated
| if err != nil { | ||
| return false, err | ||
| } | ||
| tmpEvents = append(tmpEvents, tmpEvent{lastTimestamp: lastTimestamp, reason: reason}) |
There was a problem hiding this comment.
Do not grow it anymore, all you need to do now, since it's pre-allocated is:
tmpEvents[index] = tmpEvent{lastTimestamp: lastTimestamp, reason: reason}The index variable is the one from the loop you are now discarding.
sergiordlr
force-pushed
the
MCO-1911-migrate-password-tests
branch
3 times, most recently
from
3c55863 to
2b21547
Compare
|
@sergiordlr: The following test 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-sigs/prow repository. I understand the commands that are listed here. |
|
/payload-job periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-vsphere-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-metal-ipi-ovn-ipv4-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-metal-ipi-ovn-ipv6-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-metal-ipi-ovn-dualstack-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-aws-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-gcp-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-azure-mco-disruptive-techpreview |
|
@isabella-janssen: trigger 7 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/d7d21f30-b40a-11f0-9792-c0ff0f1e4901-0 |
…tory to the MCO repository
This commit migrates all password-related tests from openshift-tests-private
to the machine-config-operator repository, enabling them to run as part of
the MCO extended test suite.
Changes:
- Added test/extended/password.go with 7 password/SSH key tests:
* PolarionID:59417 - Password create/update/delete
* PolarionID:72137 - Non-core user password validation
* PolarionID:59424 - SSH keys in new directory (RHCOS9)
* PolarionID:59426 - SSH keys update (RHCOS9)
* PolarionID:62533 - Password SSH login restriction
* PolarionID:64986 - Remove all SSH keys
* PolarionID:75552 - SSH keys with root-owned .ssh directory
- Added supporting types and utilities:
* test/extended/events.go - Event handling and custom Gomega matchers
* test/extended/types.go - Ignition 3.2 user type definition
- Enhanced existing helper modules:
* test/extended/node.go - Added GetRHELVersion(), GetDate(), CopyFromLocal(),
ExecuteDebugExpectBatch(), and event tracking methods
* test/extended/machineconfig.go - Added delete() method with MCP wait logic
* test/extended/machineconfigpool.go - Added GetCoreOsNodesOrFail() and
GetConfiguredMachineConfig() helpers
* test/extended/controller.go - Added log filtering methods
* test/extended/util.go - Added OrFail() generic utility, skipTestIfRHELVersion()
with proper version handling, MarshalOrFail(), and OCCreate()
* test/extended/remotefile.go - Complete implementation with 30+ file operations
* test/extended/generic_behaviour_validation.go - Added drain/reboot validation
and MCP degradation checks
* test/extended/const.go - Added DefaultExpectTimeout constant
- Added dependencies:
* github.com/google/goexpect - For interactive shell testing
* github.com/google/goterm - For terminal operations
* github.com/Masterminds/semver/v3 - For version comparisons
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
sergiordlr
force-pushed
the
MCO-1911-migrate-password-tests
branch
from
2b21547 to
8bcac83
Compare
|
/payload-job periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-vsphere-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-metal-ipi-ovn-ipv4-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-metal-ipi-ovn-ipv6-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-metal-ipi-ovn-dualstack-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-aws-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-gcp-mco-disruptive-techpreview periodic-ci-openshift-machine-config-operator-release-4.21-periodics-e2e-azure-mco-disruptive-techpreview |
|
@sergiordlr: trigger 7 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/e2701990-b40c-11f0-95cb-b14e44720521-0 |
There was a problem hiding this comment.
/approve
These new tests are passing in the rehearsal payload runs, minus a few of the test jobs timing out. Since the timeouts are unrelated and will need to be bumped in the short term anyways, I do not think that should block this PR. I'll leave the lgtm tagging to @pablintino since he had some more detailed change requests.
openshift-ci
bot
added
the
approved
openshift-ci-robot
added
the
verified
|
@sergiordlr: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
| event.oc.NotShowInfo() | ||
| defer event.oc.SetShowInfo() | ||
|
|
||
| lastOccurrence, err := event.GetLastTimestamp() |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: isabella-janssen, pablintino, sergiordlr 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 |
4 participants
Description
This PR migrates the MCO password test suite from the openshift-tests-private repository to the machine-config-operator repository.
Changes
New Test Files
test/extended/mco_password.go- 7 test cases for password and SSH key managementtest/extended/events.go- Event handling utilities for test validationtest/extended/types.go- Type definitions for Ignition configurationModified Files
test/extended/node.go- AddedGetRHELVersion()method for RHEL version detectiontest/extended/util.go- AddedOrFail()generic utility andskipTestIfRHELVersion()helpertest/extended/machineconfigpool.go- AddedGetCoreOsNodesOrFail()methodtest/extended/machineconfig.go- AddedGetAuthorizedKeysByUserAsList()methodtest/extended/controller.go- AddedIgnoreLogsBeforeNowOrFail()methodtest/extended/remotefile.go- AddedPushNewOwner()methodtest/extended/const.go- AddedDefaultExpectTimeoutconstanttest/extended/generic_behaviour_validation.go- AddedvalidatMcpRenderDegraded()methodDependencies
github.com/google/goexpectfor interactive session testinggithub.com/google/gotermdependencyTest Cases Migrated
Test Execution Results ✅
All 7 migrated test cases have been successfully executed and passed:
Summary
All password and SSH key management tests have been successfully migrated from the private repository and validated in the machine-config-operator repository.