Skip to content

USHIFT-7342: Replace hardcoded sleep with state check before restart#6996

Open
pacevedom wants to merge 1 commit into
openshift:mainfrom
pacevedom:test-restarts
Open

USHIFT-7342: Replace hardcoded sleep with state check before restart#6996
pacevedom wants to merge 1 commit into
openshift:mainfrom
pacevedom:test-restarts

Conversation

@pacevedom

@pacevedom pacevedom commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

The Restart MicroShift keyword had an unconditional 10s sleep before every restart to avoid issuing restarts while the service was mid-transition. Replace it with an active check that polls systemd SubState for transitional states, passing instantly when the service is already stable and saving ~10s per restart.

Summary by CodeRabbit

  • Bug Fixes
    • Improved the MicroShift restart flow by removing the fixed wait and instead waiting for the service to fully settle before restarting.
    • Reset systemd’s failed state to prevent restart rejection, then verified MicroShift and etcd restarted, refreshed kubeconfig, and confirmed readiness.
  • Tests
    • Added a new service-state check to ensure MicroShift is not transitioning (not activating, deactivating, or reloading) before restart.

@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 Jul 7, 2026
@pacevedom

Copy link
Copy Markdown
Contributor Author

/test ?

@openshift-ci

openshift-ci Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

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

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

Restart MicroShift now waits for microshift.service to leave transitional states, clears failed state before restart, tracks PID changes, and reruns kubeconfig setup and readiness checks. A new keyword checks whether microshift.service is in a transitional ActiveState.

Changes

MicroShift Restart Reliability Update

Layer / File(s) Summary
Restart flow and transition check
test/resources/microshift-process.resource
Restart MicroShift now records pre-restart PIDs, waits for microshift.service to stop transitioning, resets failed state, restarts the service, waits for new PIDs, then reruns kubeconfig setup and readiness checks; MicroShift Service Is Not Transitioning rejects activating, deactivating, and reloading states.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 15
✅ Passed checks (15 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed PR modifies only test/resources/microshift-process.resource (Robot Framework), not Ginkgo tests. Custom check for Ginkgo test name stability is not applicable.
Test Structure And Quality ✅ Passed PR modifies Robot Framework resource files, not Ginkgo tests. The custom check is designed for Ginkgo test code, which is not present in this PR, making it non-applicable.
Microshift Test Compatibility ✅ Passed The PR only changes a Robot Framework resource keyword; no new Ginkgo tests, tags, or unsupported OpenShift APIs/resources were added.
Single Node Openshift (Sno) Test Compatibility ✅ Passed Diff only updates a Robot resource keyword; no new Ginkgo tests or multi-node assumptions were added.
Topology-Aware Scheduling Compatibility ✅ Passed Only a Robot Framework test keyword was changed; no manifests, controllers, replicas, affinities, selectors, or PDBs were introduced.
Ote Binary Stdout Contract ✅ Passed PR only edits a Robot resource file; no process-level code, suite setup, or stdout writes were added, so the OTE stdout contract is unaffected.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No new Ginkgo test was added; the only change is a Robot Framework keyword in a resource file, with no IPv4 or external connectivity assumptions.
No-Weak-Crypto ✅ Passed The PR only edits a Robot test resource and adds no weak crypto, custom crypto, or secret comparisons.
Container-Privileges ✅ Passed Only change is test/resources/microshift-process.resource; no K8s/container manifests or privilege fields (privileged/hostPID/hostNetwork/etc.) are present.
No-Sensitive-Data-In-Logs ✅ Passed No new logging was added in the PR, and the existing logs only emit pidof/systemctl command output with no obvious sensitive data.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: replacing the fixed sleep with a service-state check before restart.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2026
@pacevedom

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-tests

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/resources/microshift-process.resource`:
- Around line 83-89: The PID snapshot is taken too early in the restart flow, so
the existing MicroShift process and etcd PIDs can become stale before the
restart is triggered. Move the MicroShift Process ID and MicroShift Etcd Process
ID captures in this resource so they happen after `Wait Until Keyword Succeeds`
/ `MicroShift Service Is Not Transitioning` and immediately before `Systemctl   
restart    microshift.service`, then keep the `Wait Until ... Changes` checks
using those fresh values.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 9704d8bc-0c13-4dad-86cb-b127ce30b0db

📥 Commits

Reviewing files that changed from the base of the PR and between 3b84391 and 80a91ee.

📒 Files selected for processing (1)
  • test/resources/microshift-process.resource

Comment thread test/resources/microshift-process.resource Outdated
@pacevedom

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-tests

@pacevedom

Copy link
Copy Markdown
Contributor Author

/test all

@pacevedom pacevedom changed the title Replace hardcoded sleep with state check before restart USHIFT-7342: Replace hardcoded sleep with state check before restart Jul 8, 2026
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 8, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 8, 2026

Copy link
Copy Markdown

@pacevedom: This pull request references USHIFT-7342 which is a valid jira issue.

Details

In response to this:

The Restart MicroShift keyword had an unconditional 10s sleep before every restart to avoid issuing restarts while the service was mid-transition. Replace it with an active check that polls systemd SubState for transitional states, passing instantly when the service is already stable and saving ~10s per restart.

Summary by CodeRabbit

  • Bug Fixes
  • Improved the MicroShift restart flow by removing the fixed delay and instead waiting for the service to fully settle before restarting.
  • Added handling to reset systemd’s failed state prior to restart, then verifies MicroShift and etcd restarted, refreshes kubeconfig, and confirms readiness.
  • Tests
  • Added a new service-state check to ensure MicroShift is not transitioning (not activating, deactivating, or reloading) before restart.

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.

The Restart MicroShift keyword had an unconditional 10s sleep
before every restart to avoid issuing restarts while the service
was mid-transition. Replace it with an active check that polls
systemd ActiveState for transitional states, passing instantly when
the service is already stable and saving ~10s per restart.

Also reset the systemd failure counter before restarting so that
start-limit-hit from prior rapid failures (e.g. invalid config
tests) does not block the restart.
@pacevedom pacevedom marked this pull request as ready for review July 8, 2026 08:22
@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 Jul 8, 2026
@openshift-ci openshift-ci Bot requested review from copejon and jogeo July 8, 2026 08:23
... many start requests.
[Documentation] Restart the MicroShift service, waiting for any in-progress
... state transition to settle before issuing the restart.
Wait Until Keyword Succeeds 10x 5s

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could 50s be too short? elsewhere in this same file "wait for MicroShift Service" allows up to 500

... MicroShift Service Is Not Transitioning
# Clear start-limit-hit so systemctl restart is not rejected after
# rapid failures (e.g. invalid config that was since removed).
Execute Command systemctl reset-failed microshift.service sudo=True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

now that reset-failed is introduced i wonder if transition check (wait) is redundant now

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we could also have a reutrn code check

@openshift-ci

openshift-ci Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Neilhamza, pacevedom

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

The pull request process is described here

Details 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

@pacevedom

Copy link
Copy Markdown
Contributor Author

/retest

@openshift-ci

openshift-ci Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

@pacevedom: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants