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.

Sign up for GitHub

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

Jump to bottom

Adding Status conditions for CRs #2062

Merged
merged 2 commits into from
Dec 15, 2022
Merged

Adding Status conditions for CRs #2062

merged 2 commits into from
Dec 15, 2022

Conversation

vara2504
Copy link
Contributor

@vara2504 vara2504 commented Jul 4, 2022

Description

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

@marvin-tigera marvin-tigera added this to the v1.28.0 milestone Jul 4, 2022
@vara2504 vara2504 marked this pull request as ready for review July 11, 2022 21:58
@vara2504 vara2504 requested a review from a team as a code owner July 11, 2022 21:58
@vara2504 vara2504 force-pushed the StatusConditionCR branch 2 times, most recently from 6c37102 to df1e690 Compare July 25, 2022 22:52
@vara2504 vara2504 force-pushed the StatusConditionCR branch 2 times, most recently from dcd5789 to d5272d5 Compare August 4, 2022 23:10
@mgleung mgleung modified the milestones: v1.28.0, v1.29.0 Aug 16, 2022
rene-dekker
rene-dekker requested changes Sep 19, 2022
View reviewed changes
pkg/controller/amazoncloudintegration/amazoncloudintegration_controller.go Outdated Show resolved Hide resolved
pkg/controller/status/status.go Outdated Show resolved Hide resolved
pkg/controller/applicationlayer/applicationlayer_controller.go Outdated Show resolved Hide resolved
pkg/controller/applicationlayer/applicationlayer_controller.go Outdated Show resolved Hide resolved
pkg/controller/authentication/authentication_controller_test.go Outdated Show resolved Hide resolved
pkg/controller/compliance/compliance_controller.go Outdated Show resolved Hide resolved
pkg/controller/authentication/authentication_controller.go Outdated Show resolved Hide resolved
pkg/controller/intrusiondetection/intrusiondetection_controller.go Outdated Show resolved Hide resolved
pkg/controller/logcollector/logcollector_controller.go Outdated Show resolved Hide resolved
pkg/controller/logstorage/esgateway.go Outdated Show resolved Hide resolved
@vara2504 vara2504 force-pushed the StatusConditionCR branch 3 times, most recently from 3d2945b to ee4df77 Compare September 21, 2022 17:42
rene-dekker
rene-dekker approved these changes Sep 21, 2022
View reviewed changes
Copy link
Member

@rene-dekker rene-dekker left a comment

Choose a reason for hiding this comment

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

lgtm

@vara2504 vara2504 force-pushed the StatusConditionCR branch 2 times, most recently from 427f527 to bbf2f7d Compare September 23, 2022 08:38
tmjd
tmjd reviewed Sep 23, 2022
View reviewed changes
Copy link
Member

@tmjd tmjd left a comment

Choose a reason for hiding this comment

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

I made these comments before I thought to look at what was being done before. So I'm ok if we want to ignore these new comments and address them later.
The only one I'd say we should maybe consider doing now is changing the explicitDegradedMsg type if it makes sense.

pkg/controller/status/status.go Outdated
m.lock.Lock()
defer m.lock.Unlock()
m.degraded = true
m.explicitDegradedReason = reason
m.explicitDegradedMsg = msg
m.explicitDegradedReason = string(reason)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I should have noticed this before but is there a reason to not change this explicitDegradedReason to an operator.TigeraStatusReason so then we don't need to cast it to a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pkg/controller/status/status.go Outdated
m.explicitDegradedReason = reason
m.explicitDegradedMsg = msg
m.explicitDegradedReason = string(reason)
m.explicitDegradedMsg = fmt.Sprintf("%s - Error: %s", msg, errormsg)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave out Error and just have the format string be %s: %s. I don't think Error adds anything to the message. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

pkg/controller/status/status.go Outdated
@@ -416,12 +418,17 @@ func (m *statusManager) RemoveCertificateSigningRequests(name string) {
}

// SetDegraded sets degraded state with the provided reason and message.
func (m *statusManager) SetDegraded(reason, msg string) {
func (m *statusManager) SetDegraded(reason operator.TigeraStatusReason, msg string, err error, log logr.Logger) {
log.WithValues(string(reason), msg).Error(err, string(reason))
Copy link
Member

Choose a reason for hiding this comment

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

The expectation for logr for WithValues that it is (key, value, key, value, ...).
From the docs

obj.logger = mainLogger.WithValues(
"name", obj.name, "namespace", obj.namespace)

I think this log should be switched to log.WithValues("reason", string(reason)).Error(err, string(msg)). This makes more sense to me because I feel like the reason is a category of an error and the real informational part is the msg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

rene-dekker
rene-dekker reviewed Dec 14, 2022
View reviewed changes
pkg/controller/apiserver/apiserver_controller.go Outdated
return reconcile.Result{}, err
}

var certificates []certificatemanagement.CertificateInterface
if keyValidatorConfig != nil {
dexSecret, err := certificateManager.GetCertificate(r.client, render.DexTLSSecretName, common.OperatorNamespace())
if err != nil {
reqLogger.Error(err, fmt.Sprintf("failed to retrieve %s", render.DexTLSSecretName))
r.status.SetDegraded(fmt.Sprintf("Failed to retrieve %s", render.DexTLSSecretName), err.Error())
r.status.SetDegraded(operatorv1.ResourceRenderingError, fmt.Sprintf("Failed to retrieve %s", render.DexTLSSecretName), err, reqLogger)
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be a read error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

rene-dekker
rene-dekker reviewed Dec 14, 2022
View reviewed changes
pkg/controller/logstorage/eskubecontrollers.go Outdated
return reconcile.Result{}, false, err
}

if err = imageset.ResolveImages(imageSet, esKubeControllerComponents); err != nil {
reqLogger.Error(err, "Error resolving ImageSet for elasticsearch kube-controllers components")
r.status.SetDegraded("Error resolving ImageSet for elasticsearch kube-controllers components", err.Error())
r.status.SetDegraded(operatorv1.ResourceUpdateError, "Error resolving ImageSet for elasticsearch kube-controllers components", err, reqLogger)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is also a ResourceValidationError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@vara2504
Adding Status conditions for CRs
03ff49d 
Adding Status condition changes fix

UT changes for Status condition

Adding status condition UT fixes

Adding status condition UT fixes

Build fixes

Usage comment chagnes

Moving Setdegraded to status

Fix UT for CR Condition status

Rebase CR condition status changes with master

Fix Intrusion detection UT

update setdegraded definition to Tigerareason

Rebase master

Fix UT for setdegraded definition update
rene-dekker
rene-dekker approved these changes Dec 15, 2022
View reviewed changes
Copy link
Member

@rene-dekker rene-dekker left a comment

Choose a reason for hiding this comment

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

lgtm

@rene-dekker rene-dekker merged commit fb2771e into tigera:master Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmjd tmjd tmjd left review comments

@rene-dekker rene-dekker rene-dekker approved these changes

Assignees
No one assigned
Labels
docs-pr-required release-note-required
Projects
None yet
Milestone
v1.29.0
Development

Successfully merging this pull request may close these issues.
5 participants
@vara2504 @tmjd @rene-dekker @mgleung @marvin-tigera