feat: add kstatus conforment status updates #284

Skarlso · 2023-10-31T14:20:50Z

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

What type of PR is this? (check all applicable)

Related Tickets & Documents

Related Issue # (issue)
Closes # (issue)
Fixes # (issue)

Remove if not applicable

Screenshots

Added tests?

👍 yes
🙅 no, because they aren't needed
🙋 no, because I need help
Separate ticket for tests # (issue/pr)

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Added to documentation?

📜 README.md
🙅 no documentation needed

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Thank you, but this is a draft PR and is far from over. :)

api/v1alpha1/componentversion_types.go

api/v1alpha1/condition_types.go

controllers/componentversion_controller.go

controllers/mutate_condition_status.go

souleb · 2023-11-03T15:14:24Z

controllers/register_status_defer.go

+) error {
+	// If still reconciling then reconciliation did not succeed, set to ProgressingWithRetry to
+	// indicate that reconciliation will be retried.
+	if conditions.IsReconciling(obj) {


we should also check if it's not reconciling and mark as ready.

I was thinking about that, but since we explicitly remove the reconciling condition at the end, we might as well mark it as ready by then. So each controller will take care of marking itself ready at the end.

But maybe it would be better in the defer. 🤔 I'll play around with it and see how it flows. :)

Yeah, this would require checking any potential error values. Because it might be not reconciling because it failed.

Also, I like the explicit mark. We moved away from marking it failed in defer because it was way too obscure.

It depends what is your status when you return with an error. In resources_reconciler.go you have this :

MarkAsStalled(r.EventRecorder, obj, v1alpha1.AuthenticatedContextCreationFailedReason, err.Error())

This means that you will have stalled and reconciling both true if you don't check in the defer I believe. Or you shouldmake sure that the MarkAsStalled function also unset reconciling condition.

conditions.MarkFalse(obj, meta.ReadyCondition, reason, msg) conditions.MarkStalled(obj, reason, msg)

Yah, the MarkAsStalled marks ready condition to false.

api/v1alpha1/condition_types.go

controllers/componentversion_controller.go

controllers/register_status_defer.go

Skarlso · 2023-11-08T14:44:18Z

Okay, here is an object going from Stalled to ready once the secret exists:

➜  SAP k describe componentversion -A
Name:         podinfo
Namespace:    ocm-system
Labels:       <none>
Annotations:  <none>
API Version:  delivery.ocm.software/v1alpha1
Kind:         ComponentVersion
Metadata:
  Creation Timestamp:  2023-11-08T12:31:39Z
  Generation:          1
  Resource Version:    1961
  UID:                 b6af9c07-d7a5-4c17-b7af-4e29f01930eb
Spec:
  Component:  github.com/acme/podinfo
  Interval:   1m
  Repository:
    Secret Ref:
      Name:  creds-2
    URL:     ghcr.io/Skarlso
  Version:
    Semver:  v6.0.0
Status:
  Conditions:
    Last Transition Time:  2023-11-08T12:31:40Z
    Message:               authentication failed for repository: ghcr.io/Skarlso with error: failed to configure credentials for source: failed to configure credentials for component: Secret "creds-2" not found
    Observed Generation:   1
    Reason:                AuthenticatedContextCreationFailed
    Status:                True
    Type:                  Stalled
    Last Transition Time:  2023-11-08T12:31:40Z
    Message:               authentication failed for repository: ghcr.io/Skarlso with error: failed to configure credentials for source: failed to configure credentials for component: Secret "creds-2" not found
    Observed Generation:   1
    Reason:                AuthenticatedContextCreationFailed
    Status:                False
    Type:                  Ready
Events:
  Type     Reason                              Age   From            Message
  ----     ------                              ----  ----            -------
  Warning  AuthenticatedContextCreationFailed  14m   ocm-controller  authentication failed for repository: ghcr.io/Skarlso with error: failed to configure credentials for source: failed to configure credentials for component: Secret "creds-2" not found
  Warning  AuthenticatedContextCreationFailed  4m8s  ocm-controller  authentication failed for repository: ghcr.io/Skarlso with error: failed to configure credentials for source: failed to configure credentials for component: Secret "creds-2" not found
  Warning  AuthenticatedContextCreationFailed  34s   ocm-controller  authentication failed for repository: ghcr.io/Skarlso with error: failed to configure credentials for source: failed to configure credentials for component: Secret "creds-2" not found

note: there are three events because I player around with it. :)

Then I created the secret:

Status:
  Component Descriptor:
    Component Descriptor Ref:
      Name:       github.com-acme-podinfo-v6.0.0-14336997076718420636
      Namespace:  ocm-system
    Name:         github.com/acme/podinfo
    Version:      v6.0.0
  Conditions:
    Last Transition Time:  2023-11-08T12:46:18Z
    Message:               Applied version: v6.0.0
    Observed Generation:   1
    Reason:                Succeeded
    Status:                True
    Type:                  Ready
  Observed Generation:     1
  Reconciled Version:      v6.0.0
Events:
  Type     Reason                              Age    From            Message
  ----     ------                              ----   ----            -------
  Warning  AuthenticatedContextCreationFailed  14m    ocm-controller  authentication failed for repository: ghcr.io/Skarlso with error: failed to configure credentials for source: failed to configure credentials for component: Secret "creds-2" not found
  Warning  AuthenticatedContextCreationFailed  4m31s  ocm-controller  authentication failed for repository: ghcr.io/Skarlso with error: failed to configure credentials for source: failed to configure credentials for component: Secret "creds-2" not found
  Warning  AuthenticatedContextCreationFailed  57s    ocm-controller  authentication failed for repository: ghcr.io/Skarlso with error: failed to configure credentials for source: failed to configure credentials for component: Secret "creds-2" not found
  Normal   AuthenticatedContextCreationFailed  17s    ocm-controller  Version check succeeded, found latest version: v6.0.0
  Normal   Succeeded                           12s    ocm-controller  Reconciliation finished, next run in 1m0s

The logs where just waiting, so no reconciliation was happening until the moment I created the secret. Once I removed the secret, the component version went into Stalled again. 🎉 🎉

phoban01

Just made once suggestion on the Reason question

api/v1alpha1/condition_types.go

Skarlso requested review from phoban01 and shivenduverma-sap October 31, 2023 14:20

Skarlso force-pushed the kstatus branch from 38a890c to 3f8c636 Compare October 31, 2023 14:24

shivenduverma-sap previously approved these changes Nov 2, 2023

View reviewed changes

Skarlso force-pushed the kstatus branch 2 times, most recently from 18ffbe0 to 7f078d1 Compare November 3, 2023 09:08

Skarlso marked this pull request as ready for review November 3, 2023 09:22

Skarlso requested a review from souleb November 3, 2023 09:22

robertwol linked an issue Nov 3, 2023 that may be closed by this pull request

Kstatus compatibility #216

Closed

souleb reviewed Nov 3, 2023

View reviewed changes

Skarlso force-pushed the kstatus branch from c69054d to 7343553 Compare November 6, 2023 07:10

Skarlso requested a review from souleb November 6, 2023 12:58

Skarlso force-pushed the kstatus branch from d4d89ee to 05d768d Compare November 7, 2023 16:38

phoban01 reviewed Nov 7, 2023

View reviewed changes

api/v1alpha1/condition_types.go Show resolved Hide resolved

controllers/componentversion_controller.go Show resolved Hide resolved

controllers/register_status_defer.go Outdated Show resolved Hide resolved

Skarlso force-pushed the kstatus branch from 4a92f31 to c7b07d1 Compare November 8, 2023 14:40

Skarlso requested a review from phoban01 November 8, 2023 16:43

phoban01 approved these changes Nov 10, 2023

View reviewed changes

api/v1alpha1/condition_types.go Show resolved Hide resolved

Skarlso added 7 commits November 14, 2023 11:24

feat: add kstatus conforment status updates

9601698

adding applying status update to configuration controller

6066c9f

removed progressive status from error cases

78fad04

remove superflous object generation set

ef51cbb

fix the e2e test and rename some functions

f83ea98

adding reconciliation and stalling on missing authentication information

014c4b5

removed the interface as it was not needed

1301b28

Skarlso force-pushed the kstatus branch from c7b07d1 to 1301b28 Compare November 14, 2023 10:27

extracted status update logic and removed hardcoded statuses

2893b28

Skarlso force-pushed the kstatus branch from 48b6819 to 2893b28 Compare November 14, 2023 11:12

Skarlso merged commit 5a95373 into main Nov 14, 2023
5 checks passed

Skarlso deleted the kstatus branch November 14, 2023 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add kstatus conforment status updates #284

feat: add kstatus conforment status updates #284

Skarlso commented Oct 31, 2023

souleb Nov 3, 2023

Skarlso Nov 6, 2023

Skarlso Nov 6, 2023

Skarlso Nov 8, 2023

souleb Nov 14, 2023

Skarlso Nov 14, 2023

Skarlso commented Nov 8, 2023 •

edited

Loading

phoban01 left a comment

feat: add kstatus conforment status updates #284

feat: add kstatus conforment status updates #284

Conversation

Skarlso commented Oct 31, 2023

Description

What type of PR is this? (check all applicable)

Related Tickets & Documents

Screenshots

Added tests?

Added to documentation?

Checklist:

souleb Nov 3, 2023

Choose a reason for hiding this comment

Skarlso Nov 6, 2023

Choose a reason for hiding this comment

Skarlso Nov 6, 2023

Choose a reason for hiding this comment

Skarlso Nov 8, 2023

Choose a reason for hiding this comment

souleb Nov 14, 2023

Choose a reason for hiding this comment

Skarlso Nov 14, 2023

Choose a reason for hiding this comment

Skarlso commented Nov 8, 2023 • edited Loading

phoban01 left a comment

Choose a reason for hiding this comment

Skarlso commented Nov 8, 2023 •

edited

Loading