-
Notifications
You must be signed in to change notification settings - Fork 24
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
CMP-2132: Implement suspend and resume scan schedule #396
Conversation
@rhmdnd: This pull request references CMP-2132 which is a valid jira issue. 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/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -9,7 +9,12 @@ Versioning](https://semver.org/spec/v2.0.0.html). | |||
|
|||
### Enhancements | |||
|
|||
- | |||
- Users can now pause scan schedules by setting the `ScanSetting.suspend` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a great QOL feature to add! I'll be sure it's highlighted in the release notes downstream in the future.
e8f84a3
to
25f04d3
Compare
tests/e2e/parallel/main_test.go
Outdated
|
||
} | ||
|
||
func TestSuspendScanSettingDoesNotCreateScan(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should move these to serial tests since they will fail if they're run at the same time as another test that's using ocp4-cis
or ocp4-cis-node
.
@xiaojiey I have one minor test fix to make, but otherwise these should be good to go for pre-verification. |
// ScanSetting.suspend attribute is disabled, or set to False, the | ||
// ComplianceSuite will get updated and schedule scans. | ||
if suite.Spec.Suspend { | ||
return false, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the ComplianceSuite
's phase (status) should be changed to something like SUSPENDED
.
For example:
$ oc get compliancesuites
NAME PHASE RESULT
nist-moderate RUNNING NOT-AVAILABLE
cis SUSPENDED NON-COMPLIANT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea - I'll see if I can incorporate that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got down the path of incorporating status into the ComplianceSuite, and realized that having a Suspended status there makes reconciling it when the ScanSetting
is reactivated much more complicated.
I'll propose an alternative with the status on the ScanSettingBinding
, which should allow us to relay the same information, but it makes it simpler to reconcile because we can short-circuit the ComplianceScan
creation if we have a binding that references a suspended scan setting.
tests/e2e/parallel/main_test.go
Outdated
} | ||
if *job.Spec.Suspend == false { | ||
t.Fatalf("Expected CronJob %s to be suspended", jobName) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we add SUSPENDED
Status/Phase, then here we would also check for the status to be SUSPENDED
withWaitForSuiteScansStatus()
.
tests/e2e/parallel/main_test.go
Outdated
err := f.AssertScanDoesNotExist(scanName, f.OperatorNamespace) | ||
if err != nil { | ||
t.Fatal(err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well, check the status of the ComplianceSuite.
@rhmdnd Generally it is quite good.
|
@rhmdnd Retest the PR, the suite will be updated to "SUSPENDED" phase when the suspend enabled. ##########Create a ss with suspend set to true, then create a ssb and unsuspend the ssb on demand:
|
@@ -157,7 +164,10 @@ func (r *ReconcileComplianceSuite) Reconcile(ctx context.Context, request reconc | |||
return reconcile.Result{}, nil | |||
} | |||
|
|||
suiteCopy := suite.DeepCopy() | |||
suiteCopy, err := r.refreshSuite(suite) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this complexity if we have the status on the binding.
@yuumasato @Vincent056 should be ready for another round of reviews. |
pkg/controller/scansettingbinding/scansettingbinding_controller.go
Outdated
Show resolved
Hide resolved
8170852
to
34bc4e6
Compare
@xiaojiey @yuumasato @Vincent056 I cleaned up all remaining comments and this should be ready for another round of reviews. |
@@ -293,6 +293,53 @@ or `ocp4-var-role-worker`: | |||
`oc get ccr -n openshift-compliance -o yaml | jq '.items[] | select(.valuesUsed | contains("ocp4-var-role-master") or contains("ocp4-var-role-worker"))'` | |||
|
|||
|
|||
## Suspending and resuming scan schedules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is excellent. Thanks @rhmdnd !
pkg/controller/scansettingbinding/scansettingbinding_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/scansettingbinding/scansettingbinding_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/scansettingbinding/scansettingbinding_controller.go
Outdated
Show resolved
Hide resolved
@rhmdnd: This pull request references CMP-2132 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.15.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 kubernetes/test-infra repository. |
pkg/controller/scansettingbinding/scansettingbinding_controller.go
Outdated
Show resolved
Hide resolved
This commit implements the logic and tests necessary to suspend and resume scan schedules using the `ScanSetting` custom resource. You can find more details on the overall justification, use cases, and implementation details in the enhancement: ComplianceAsCode#375
Retest with 4.14.0-0.nightly-2023-10-06-234925 + code in the PR. Verification pass.
|
/unhold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for addressing all the comments,
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhmdnd, Vincent056 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 |
This commit implements the logic and tests necessary to suspend and
resume scan schedules using the
ScanSetting
custom resource.You can find more details on the overall justification, use cases, and
implementation details in the enhancement:
#375