MGMT-22782: Add state root cleanup script to day-2 worker ignition#9942
MGMT-22782: Add state root cleanup script to day-2 worker ignition#9942CrystalChun wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
@CrystalChun: This pull request references MGMT-22782 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 bug to target the "4.22.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. |
openshift-ci
bot
added
do-not-merge/work-in-progress
WalkthroughAdds two exported ignition utilities and a cleanup ignition constant in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun 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 |
openshift-ci
bot
added
the
approved
|
@CrystalChun: This pull request references MGMT-22782 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 bug to target the "4.22.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/ignition/common.go (1)
61-64: Preserve the root cause when hostname lookup fails.The current return path drops the underlying
err, which makes failures harder to debug.Proposed fix
hostname, err := hostutil.GetCurrentHostName(host) if err != nil { - return nil, errors.Errorf("failed to get hostname for host %s", host.ID) + return nil, errors.Wrapf(err, "failed to get hostname for host %s", host.ID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ignition/common.go` around lines 61 - 64, The error returned from hostutil.GetCurrentHostName is being discarded; update the failure return in the block handling GetCurrentHostName to preserve and wrap the original err (e.g., use errors.Wrapf(err, "failed to get hostname for host %s", host.ID) or fmt.Errorf("failed to get hostname for host %s: %w", host.ID, err)) so callers see the root cause; look for the hostname lookup call to hostutil.GetCurrentHostName in internal/ignition/common.go and change the return that currently calls errors.Errorf to wrap the underlying err.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ignition/common.go`:
- Around line 7-10: The imports in common.go are incorrect: replace imports of
"github.com/openshift/assisted-service/internal/ignition/ignitioncommon" and
"github.com/openshift/assisted-service/internal/models" with the
project-standard packages
"github.com/openshift/assisted-service/internal/common/ignition" and
"github.com/openshift/assisted-service/models" so the helper functions
ParseToLatest, SetFileInIgnition, MergeIgnitionConfig and the GetCurrentHostName
signature (which expects *models.Host) match the rest of the codebase and avoid
type/import mismatches; update the import block accordingly.
---
Nitpick comments:
In `@internal/ignition/common.go`:
- Around line 61-64: The error returned from hostutil.GetCurrentHostName is
being discarded; update the failure return in the block handling
GetCurrentHostName to preserve and wrap the original err (e.g., use
errors.Wrapf(err, "failed to get hostname for host %s", host.ID) or
fmt.Errorf("failed to get hostname for host %s: %w", host.ID, err)) so callers
see the root cause; look for the hostname lookup call to
hostutil.GetCurrentHostName in internal/ignition/common.go and change the return
that currently calls errors.Errorf to wrap the underlying err.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
internal/ignition/common.gointernal/ignition/discovery.gointernal/ignition/installmanifests.go
CrystalChun
force-pushed
the
day-2-capoa
branch
from
dc8f270 to
2011b32
Compare
|
@CrystalChun: This pull request references MGMT-22782 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 bug to target the "4.22.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. |
Adding day-2 hosts originally skips adding this cleanup script even though the day-2 host also is booting from a persistent disk. We should ensure that both day-1 and day-2 hosts that are booting from a disk image get this script.
CrystalChun
force-pushed
the
day-2-capoa
branch
from
2011b32 to
f2918cb
Compare
|
@CrystalChun: This pull request references MGMT-22782 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 bug to target the "4.22.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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/ignition/common.go (1)
61-64: Original error is discarded in error message.The error from
GetCurrentHostNameis retrieved but not included in the returned error message. This could make debugging harder when hostname retrieval fails.♻️ Suggested fix to include original error
hostname, err := hostutil.GetCurrentHostName(host) if err != nil { - return nil, errors.Errorf("failed to get hostname for host %s", host.ID) + return nil, errors.Wrapf(err, "failed to get hostname for host %s", host.ID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ignition/common.go` around lines 61 - 64, The returned error discards the original error from hostutil.GetCurrentHostName; update the error return to include/wrap the original err (e.g. use errors.Wrapf(err, "failed to get hostname for host %s", host.ID) or fmt.Errorf("failed to get hostname for host %s: %w", host.ID, err)) so the original error context from GetCurrentHostName is preserved; locate the hostname, err := hostutil.GetCurrentHostName(host) block and replace the current errors.Errorf return accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/ignition/common.go`:
- Around line 61-64: The returned error discards the original error from
hostutil.GetCurrentHostName; update the error return to include/wrap the
original err (e.g. use errors.Wrapf(err, "failed to get hostname for host %s",
host.ID) or fmt.Errorf("failed to get hostname for host %s: %w", host.ID, err))
so the original error context from GetCurrentHostName is preserved; locate the
hostname, err := hostutil.GetCurrentHostName(host) block and replace the current
errors.Errorf return accordingly.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
internal/ignition/common.gointernal/ignition/discovery.gointernal/ignition/discovery_test.gointernal/ignition/installmanifests.gointernal/ignition/templates/node.ign
✅ Files skipped from review due to trivial changes (1)
- internal/ignition/templates/node.ign
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/ignition/installmanifests.go
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9942 +/- ##
=======================================
Coverage 44.06% 44.07%
=======================================
Files 414 415 +1
Lines 72172 72218 +46
=======================================
+ Hits 31803 31828 +25
- Misses 37494 37512 +18
- Partials 2875 2878 +3
🚀 New features to boost your workflow:
|
|
/cc @rccrdpccl |
|
/test edge-subsystem-kubeapi-aws |
|
@CrystalChun: all tests passed! 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. |
2 participants
Adding day-2 hosts originally skips adding this cleanup script even though the day-2 host also is booting from a persistent disk.
We should ensure that both day-1 and day-2 hosts that are booting from a disk image get this script.
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit
New Features
Refactor
Tests
Chore