fixes #1258 controller updates deployment when headless boolean is up…

[Horiodino]

Description of Changes

checks if the environment variable "REGISTRY_HEADLESS" exists in headlessStatus, parses its value to a boolean, and sets needsUpdating to true if the parsed value differs from the Headless spec in the cr.Spec

Related Issue(s)

devfile/api#1258

Acceptance Criteria

Tests

• Test Coverage
  • Are your changes sufficiently tested, and are any applicable test cases added or updated to cover your changes?
• Gosec scans
  • Fix all MEDIUM and higher findings and/or annotate a finding per gosec instructions: https://github.com/securego/gosec#annotating-code to address why a finding is not a security issue

Documentation

• [ ✔️ ] Does the registry operator documentation need to be updated with your changes?

Tests Performed

Explain what tests you personally ran to ensure the changes are functioning as expected.

How To Test

Instructions for the reviewer on how to test your changes.

Running Unit Tests

Running Integration Tests

Notes To Reviewer

Any notes you would like to include for the reviewer.

[openshift-ci]

Hi @Horiodino. Thanks for your PR.

I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[Jdubrick]

/ok-to-test

[Jdubrick]

Is it possible to rename this var to be more clear that it is the array of all env vars for the container?

[Jdubrick]

Not able to get it to update when the headless is altered. Currently deploying with

apiVersion: registry.devfile.io/v1alpha1
kind: DevfileRegistry
metadata:
  name: devfile-registry
spec:
  telemetry:
    registryName: test
  k8s:
    ingressDomain: testing-registry
  tls:
    enabled: false
  headless: true

and when running kubectl apply -n devfile registry -f <path to yaml> with this file:

apiVersion: registry.devfile.io/v1alpha1
kind: DevfileRegistry
metadata:
  name: devfile-registry
spec:
  telemetry:
    registryName: test
  k8s:
    ingressDomain: testing-registry
  tls:
    enabled: false
  headless: false

It does not update. I also noticed that if you remove headless altogether then it segfaults as a memory issue, could be a one off but worth looking into.

Was this working for you/was it tested? @michael-valdron @thepetk have either of you been able to reproduce?

[thepetk]

Left a comment to get more info. I think the current feature needs to have some test cases too.

[Jdubrick]

I am still unable to get this to spin up/spin down the viewer depending on the value of the headless env var. If I deploy with headless: true and then re-apply after with headless: false it does not spin up the viewer and vice-versa.

You are able to verify this by applying and deploying everything to Minikube/Kind, @Horiodino have you done this?

@thepetk do you experience the same things?

[thepetk]

I am still unable to get this to spin up/spin down the viewer depending on the value of the headless env var. If I deploy with headless: true and then re-apply after with headless: false it does not spin up the viewer and vice-versa.

You are able to verify this by applying and deploying everything to Minikube/Kind, @Horiodino have you done this?

@thepetk do you experience the same things?

I feel that's because we're currently expecting that an env var is set for that purpose. I agree tho what @Jdubrick is at least my idea of the expected behavior

[Jdubrick]

I am still unable to get this to spin up/spin down the viewer depending on the value of the headless env var. If I deploy with headless: true and then re-apply after with headless: false it does not spin up the viewer and vice-versa.
You are able to verify this by applying and deploying everything to Minikube/Kind, @Horiodino have you done this?
@thepetk do you experience the same things?

I feel that's because we're currently expecting that an env var is set for that purpose. I agree tho what @Jdubrick is at least my idea of the expected behavior

The var is present in the index container but only if headless: true or headless: false is set initially, if it isn't in the yaml file at all it isn't present. But then when you try and reapply to change it the operator doesn't pick it up. Could be related to the way the reconcile loop is working and it isn't paying attention to that field. I wonder if adding the headless field to one of the managed that triggers a rollout of the deployment would fix this

[Jdubrick]

After taking a look at this again, I believe the following is needed to get this working:

1. Updated Unit Tests
   I would personally like to see the tests have more comprehensive coverage. What I mean by that is the tests should handle the case where a Deployment has it's headless value changed after deployment and the appropriate response (viewer container is added/removed from pod) is taken. We would need to mock up the Deployment to do this in the tests.

2. Add Deployment Update Logic
   We are grabbing the headless value but I believe nothing is happening because we are relying on Update() to do this for us, but I believe in order for that to work we need to physically alter the Deployment to include/exclude that viewer container based on the headless status. This would most likely include looking through at how the viewer container is added to the deployment normally (when the headless is false) and using that as a guide to either include/exclude it when headless changes.

cc @thepetk @Horiodino

[Horiodino]

After taking a look at this again, I believe the following is needed to get this working:

1. Updated Unit Tests
   I would personally like to see the tests have more comprehensive coverage. What I mean by that is the tests should handle the case where a Deployment has it's `headless` value changed after deployment and the appropriate response (viewer container is added/removed from pod) is taken. We would need to mock up the Deployment to do this in the tests.

2. Add Deployment Update Logic
   We are grabbing the `headless` value but I believe nothing is happening because we are relying on `Update()` to do this for us, but I believe in order for that to work we need to physically alter the Deployment to include/exclude that viewer container based on the `headless` status. This would most likely include looking through at how the viewer container is added to the deployment normally (when the headless is false) and using that as a guide to either include/exclude it when headless changes.

cc @thepetk @Horiodino

Thanks for the suggestions! I’ve made some changes and will now write the test case. 👍

[Horiodino]

I believe there's an issue with the logic we're implementing regarding the deployment process. Specifically, the problem lies in how the generateDeployment function and the update function are interacting with the CR (Custom Resource) and the deployment spec.

Here’s the problem in more detail:

• Generate Function:
  The generateDeployment function is responsible for generating the deployment based on the current state of the CR. When the CR is set to true, it checks the headless field. If headless is true, it sets it to true in the deployment spec and returns. This part seems to work fine.

• Update Function:
  The issue arises when the update function is called. During the update process, we are trying to compare the current state of the CR with the current deployment spec. The challenge here is that the CR value may have changed, but we don’t have a previous value to compare it against.

• For example:

  If the CR is set to true, and the generateDeployment function sets the headless field to true in the deployment, everything looks good.
  But when it comes to updating, the system sees that the CR is true, but in the deployment spec (dep.spec.env), the value might already be true. So, we end up with two true values, and we cannot compare them directly.
  If the CR is set to false, it won’t add the env field. However, during the update, it checks the deployment spec, and since dep.spec.env is either false or nil, there’s no clear way to determine if the value should be updated or not.

We are essentially setting the deployment based on the current value of headless, but without knowing the previous value. This lack of historical context means we can't reliably compare the current CR value with the deployment spec during updates.

We need to track and compare the previous and current values of the CR and deployment spec to ensure updates are applied correctly.

Let me know your thoughts on this.

[Jdubrick]

I believe there's an issue with the logic we're implementing regarding the deployment process. Specifically, the problem lies in how the generateDeployment function and the update function are interacting with the CR (Custom Resource) and the deployment spec.

Here’s the problem in more detail:

• Generate Function:
  The generateDeployment function is responsible for generating the deployment based on the current state of the CR. When the CR is set to true, it checks the headless field. If headless is true, it sets it to true in the deployment spec and returns. This part seems to work fine.
• Update Function:
  The issue arises when the update function is called. During the update process, we are trying to compare the current state of the CR with the current deployment spec. The challenge here is that the CR value may have changed, but we don’t have a previous value to compare it against.
• For example:
  If the CR is set to true, and the generateDeployment function sets the headless field to true in the deployment, everything looks good.
  But when it comes to updating, the system sees that the CR is true, but in the deployment spec (dep.spec.env), the value might already be true. So, we end up with two true values, and we cannot compare them directly.
  If the CR is set to false, it won’t add the env field. However, during the update, it checks the deployment spec, and since dep.spec.env is either false or nil, there’s no clear way to determine if the value should be updated or not.

We are essentially setting the deployment based on the current value of headless, but without knowing the previous value. This lack of historical context means we can't reliably compare the current CR value with the deployment spec during updates.

We need to track and compare the previous and current values of the CR and deployment spec to ensure updates are applied correctly.

Let me know your thoughts on this.

I don't believe we need to keep track of the previous value of headless, we could probably skirt around needing to keep track of the previous value by using the Reconcile properly - What I mean by that is we don't need to know the previous value of headless, just the current. If headless is false the Reconcile loop should be making sure that the viewer is spun up. If headless is true the Reconcile loop should be ensuring the viewer is not present. That way we don't need to check for if headless changed, just what the current value is.

@thepetk @Horiodino

[Horiodino]

the test case runs fine for now , but im not able to run locally , OOM my pc cant handle 😅

[Jdubrick]

Looks good to me now, I was able to verify that it will spin up or tear down the viewer depending on the new value of headless in the CR.

@Horiodino are you able to run go mod tidy and push those changes? I believe that is why the one CI check is failing

[Jdubrick]

/retest

[Jdubrick]

@Horiodino I think the tests are failing because the new file you added for the tests is missing the copyright header, you can see the header present in update.go but missing in update_test.go for example. Would you mind adding that as part of this PR to verify if it is the issue? We'd need the header anyways

[Jdubrick]

lgtm. @thepetk since you also reviewed I'll want you to sign off too

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Horiodino, Jdubrick

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:

• OWNERS [Jdubrick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[thepetk]

Looks good to me, based also on @Jdubrick feedback. I've left a minor comment for the usage of registry-viewer "hardcoded" in the code. I feel using a variable there would be better, just to ensure that we are checking everywhere for the same name.

[thepetk]

Might be better to have a static variable here for the name just to be sure we are using the same name everywhere.

[openshift-ci]

New changes are detected. LGTM label has been removed.