Refactor deployment.go code #365

bshephar · 2024-09-19T00:26:54Z

This change does some refactoring of the deployment.go code to avoid overuse of string literals and replace with consts. Along with moving excessive code from the main Deployment function into dedicated functions that are easier to test and read.

The second commit adds a dedicated deployment_test.go file. This will allow us to perform more comprehensive testing of the individual functions without depending on the limited number of scenarios we're running with envtest.

openshift-ci · 2024-09-19T00:27:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bshephar

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~~ [bshephar]

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

bshephar · 2024-09-21T08:43:30Z

/test horizon-operator-build-deploy-kuttl

bshephar · 2025-01-09T00:20:52Z

Seems to be an issue with the VolumeMounts:

    case.go:380: resource Horizon:horizon-kuttl-tests/horizon: .status.readyCount: key is missing from map
    logger.go:42: 12:27:14 | tls-deployment | skipping kubernetes event logging

Failure is related to the missing combined-ca-bundle mount:

❯ curl -s "https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openstack-k8s-operators_horizon-operator/365/pull-ci-openstack-k8s-operators-horizon-operator-main-horizon-operator-build-deploy-kuttl/1876224488794755072/artifacts/horizon-operator-build-deploy-kuttl/openstack-k8s-operators-gather/artifacts/must-gather/quay-io-openstack-k8s-operators-openstack-must-gather-sha256-b14292f8b5bb013eb06a54a9a50a2d892f6dd255901a7842f65567beb8cd090d/namespaces/horizon-kuttl-tests/crs/horizons.horizon.openstack.org/horizon.yaml" | yq '.status.conditions[0]'
lastTransitionTime: "2025-01-06T12:27:14Z"
message: 'TLSInput is missing: combined-ca-bundle'
reason: Requested
severity: Info
status: "False"
type: Ready

bshephar · 2025-02-06T23:42:02Z

/test horizon-operator-build-deploy-kuttl

bshephar · 2025-02-07T00:17:52Z

/test horizon-operator-build-deploy-kuttl

bshephar · 2025-02-07T00:44:31Z

/test functional

bshephar · 2025-02-07T04:34:27Z

Hmm, still complains that the secret is missing:

  Warning  FailedMount     112s                   kubelet            MountVolume.SetUp failed for volume "combined-ca-bundle" : secret "combined-ca-bundle" not found
  Warning  FailedMount     112s                   kubelet            MountVolume.SetUp failed for volume "horizon-tls-certs" : secret "cert-horizon-svc" not found

❯ curl -s "https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openstack-k8s-operators_horizon-operator/365/pull-ci-openstack-k8s-operators-horizon-operator-main-horizon-operator-build-deploy-kuttl/1887657005825396736/artifacts/horizon-operator-build-deploy-kuttl/openstack-k8s-operators-gather/artifacts/must-gather/quay-io-openstack-k8s-operators-openstack-must-gather-sha256-125ca476d114b0e95cbbb8df0485c867fef29386646a6631e95e7a18e1eeb85c/namespaces/horizon-kuttl-tests/crs/horizons.horizon.openstack.org/horizon.yaml" | yq ".status.conditions[0]"
lastTransitionTime: "2025-02-07T01:26:12Z"
message: 'TLSInput is missing: combined-ca-bundle'
reason: Requested
severity: Info
status: "False"
type: Ready

I was hoping we might be able to catch this with the functional test I added. But I might be misunderstanding the mechanism responsible for creating the ca-bundle.

It should be created by:
https://github.com/openstack-k8s-operators/horizon-operator/blob/main/tests/kuttl/tests/tls-deployment/00-tls_ca_bundle.yaml

So, I'm not too sure why this change results in it being missing.

❯ curl -s "https://storage.googleapis.com/test-platform-results/pr-logs/pull/openstack-k8s-operators_horizon-operator/365/pull-ci-openstack-k8s-operators-horizon-operator-main-horizon-operator-build-deploy-kuttl/1887657005825396736/build-log.txt" | grep "combined-ca-bundle
created"
    logger.go:42: 01:23:09 | tls-deployment/0-tls_ca_bundle | Secret:horizon-kuttl-tests/combined-ca-bundle created

It's possible the must-gather is being collected too late I guess. Probably easier to find this issue in e2e tests.

stuggi · 2025-02-07T08:56:16Z

But it seems not to be only a tls issue, also the basic test is not successful, its just not stopping and continue also the tls test

    logger.go:42: 01:23:07 | basic-deployment/1-deploy-horizon | + RETURN_CODE=0
    logger.go:42: 01:23:07 | basic-deployment/1-deploy-horizon | ++ oc get -n horizon-kuttl-tests horizon horizon -o 'jsonpath={.status.endpoint}'
    logger.go:42: 01:23:07 | basic-deployment/1-deploy-horizon | + PUBLIC_URL=http://horizon.horizon-kuttl-tests.svc
    logger.go:42: 01:23:07 | basic-deployment/1-deploy-horizon | ++ oc -n horizon-kuttl-tests exec -it deployment/horizon -- curl --silent --output /dev/stderr --head --write-out '%{http_code}' 'http://horizon.horizon-kuttl-tests.svc/dashboard/auth/login/?next=/dashboard/' -k
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | Unable to use a TTY - input is not a terminal or the right kind of file
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | command terminated with exit code 7
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | + STATUSCODE=000
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | + [[ -n 000 ]]
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | + test 000 -ne 200
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | + RETURN_CODE=1
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | + echo 'http://horizon.horizon-kuttl-tests.svc status code expected is 200 but was 000'
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | http://horizon.horizon-kuttl-tests.svc status code expected is 200 but was 000
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | + exit 1
    logger.go:42: 01:23:09 | basic-deployment/1-deploy-horizon | test step failed 1-deploy-horizon
    case.go:378: failed in step 1-deploy-horizon
    case.go:380: --- Horizon:horizon-kuttl-tests/horizon
        +++ Horizon:horizon-kuttl-tests/horizon
        @@ -1,14 +1,31 @@
         apiVersion: horizon.openstack.org/v1beta1
         kind: Horizon
         metadata:
        +  annotations:
        +    kubectl.kubernetes.io/last-applied-configuration: |
        +      {"apiVersion":"horizon.openstack.org/v1beta1","kind":"Horizon","metadata":{"annotations":{},"name":"horizon","namespace":"horizon-kuttl-tests"},"spec":{"customServiceConfig":"DEBUG = True\nLOGGING['handlers']['console']['level'] = 'DEBUG'\n","replicas":1,"secret":"osp-secret"}}
        +  finalizers:
        +  - openstack.org/horizon
        +  managedFields: '[... elided field over 10 lines long ...]'
           name: horizon
           namespace: horizon-kuttl-tests
         spec:
        +  containerImage: quay.io/podified-antelope-centos9/openstack-horizon@sha256:7b8817f2ee32bd77d6ec38524cfbc12c060966bb5c48c0ba04d6d21e401ee2c5
           customServiceConfig: |
             DEBUG = True
             LOGGING['handlers']['console']['level'] = 'DEBUG'
        +  extraMounts: []
        +  memcachedInstance: memcached
        +  override: {}
        +  preserveJobs: false
           replicas: 1
        +  resources: {}
           secret: osp-secret
        +  tls: {}
         status:
        -  readyCount: 1
        +  conditions: '[... elided field over 10 lines long ...]'
        +  endpoint: http://horizon.horizon-kuttl-tests.svc
        +  hash:
        +    input: nbfh678h9ch587h69h687h74h675h67bh57bh59h67h55ch695h96h95h5c5h655h5c5h59bhb5h54chcbhf5hc4h9ch587h8dh67fhcbh559h68dq
        +  observedGeneration: 1
         
        
    case.go:380: resource Horizon:horizon-kuttl-tests/horizon: .status.readyCount: key is missing from map
    case.go:380: command "RETURN_CODE=0\\n PUBLIC_URL=$(oc get -n $NAMESPACE horizon horizon -..." failed, exit status 1

Bot kuttl tests fail

--- FAIL: kuttl (364.73s)
    --- FAIL: kuttl/harness (0.00s)
        --- FAIL: kuttl/harness/basic-deployment (181.42s)
        --- FAIL: kuttl/harness/tls-deployment (183.16s)

bshephar · 2025-02-10T12:06:03Z

/test horizon-operator-build-deploy-kuttl

bshephar · 2025-02-11T04:13:43Z

But it seems not to be only a tls issue, also the basic test is not successful, its just not stopping and continue also the tls test

    logger.go:42: 01:23:07 | basic-deployment/1-deploy-horizon | + RETURN_CODE=0
    logger.go:42: 01:23:07 | basic-deployment/1-deploy-horizon | ++ oc get -n horizon-kuttl-tests horizon horizon -o 'jsonpath={.status.endpoint}'
    logger.go:42: 01:23:07 | basic-deployment/1-deploy-horizon | + PUBLIC_URL=http://horizon.horizon-kuttl-tests.svc
    logger.go:42: 01:23:07 | basic-deployment/1-deploy-horizon | ++ oc -n horizon-kuttl-tests exec -it deployment/horizon -- curl --silent --output /dev/stderr --head --write-out '%{http_code}' 'http://horizon.horizon-kuttl-tests.svc/dashboard/auth/login/?next=/dashboard/' -k
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | Unable to use a TTY - input is not a terminal or the right kind of file
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | command terminated with exit code 7
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | + STATUSCODE=000
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | + [[ -n 000 ]]
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | + test 000 -ne 200
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | + RETURN_CODE=1
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | + echo 'http://horizon.horizon-kuttl-tests.svc status code expected is 200 but was 000'
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | http://horizon.horizon-kuttl-tests.svc status code expected is 200 but was 000
    logger.go:42: 01:23:08 | basic-deployment/1-deploy-horizon | + exit 1
    logger.go:42: 01:23:09 | basic-deployment/1-deploy-horizon | test step failed 1-deploy-horizon
    case.go:378: failed in step 1-deploy-horizon
    case.go:380: --- Horizon:horizon-kuttl-tests/horizon
        +++ Horizon:horizon-kuttl-tests/horizon
        @@ -1,14 +1,31 @@
         apiVersion: horizon.openstack.org/v1beta1
         kind: Horizon
         metadata:
        +  annotations:
        +    kubectl.kubernetes.io/last-applied-configuration: |
        +      {"apiVersion":"horizon.openstack.org/v1beta1","kind":"Horizon","metadata":{"annotations":{},"name":"horizon","namespace":"horizon-kuttl-tests"},"spec":{"customServiceConfig":"DEBUG = True\nLOGGING['handlers']['console']['level'] = 'DEBUG'\n","replicas":1,"secret":"osp-secret"}}
        +  finalizers:
        +  - openstack.org/horizon
        +  managedFields: '[... elided field over 10 lines long ...]'
           name: horizon
           namespace: horizon-kuttl-tests
         spec:
        +  containerImage: quay.io/podified-antelope-centos9/openstack-horizon@sha256:7b8817f2ee32bd77d6ec38524cfbc12c060966bb5c48c0ba04d6d21e401ee2c5
           customServiceConfig: |
             DEBUG = True
             LOGGING['handlers']['console']['level'] = 'DEBUG'
        +  extraMounts: []
        +  memcachedInstance: memcached
        +  override: {}
        +  preserveJobs: false
           replicas: 1
        +  resources: {}
           secret: osp-secret
        +  tls: {}
         status:
        -  readyCount: 1
        +  conditions: '[... elided field over 10 lines long ...]'
        +  endpoint: http://horizon.horizon-kuttl-tests.svc
        +  hash:
        +    input: nbfh678h9ch587h69h687h74h675h67bh57bh59h67h55ch695h96h95h5c5h655h5c5h59bhb5h54chcbhf5hc4h9ch587h8dh67fhcbh559h68dq
        +  observedGeneration: 1
         
        
    case.go:380: resource Horizon:horizon-kuttl-tests/horizon: .status.readyCount: key is missing from map
    case.go:380: command "RETURN_CODE=0\\n PUBLIC_URL=$(oc get -n $NAMESPACE horizon horizon -..." failed, exit status 1

Bot kuttl tests fail

--- FAIL: kuttl (364.73s)
    --- FAIL: kuttl/harness (0.00s)
        --- FAIL: kuttl/harness/basic-deployment (181.42s)
        --- FAIL: kuttl/harness/tls-deployment (183.16s)

Yeah, there's something else going on. Maybe it's the probes, I should look closer at those probes:

  Warning  Unhealthy       4m18s                kubelet            Startup probe failed: Get "https://10.128.0.128:443/dashboard/auth/login/?next=/dashboard/": dial tcp 10.128.0.128:443: connect: connection refused
  Warning  Unhealthy       98s (x18 over 4m6s)  kubelet            Readiness probe failed: HTTP probe failed with statuscode: 404

pkg/horizon/deployment.go

bshephar · 2025-02-13T06:26:16Z

/test horizon-operator-build-deploy-kuttl

bshephar · 2025-02-13T06:47:39Z

/test horizon-operator-build-deploy-kuttl
/test functional

bshephar · 2025-02-14T01:50:47Z

/test horizon-operator-build-deploy-kuttl

This change does some refactoring of the deployment.go code to avoid overuse of string literals and replace with consts. Along with moving excessive code from the main Deployment function into dedicated functions that are easier to test and read. Signed-off-by: Brendan Shephard <[email protected]>

Signed-off-by: Brendan Shephard <[email protected]>

openshift-ci bot requested review from lewisdenny and viroel September 19, 2024 00:26

openshift-ci bot added the approved label Sep 19, 2024

bshephar force-pushed the refactor-cleanup branch from 574ee2a to fe0fd28 Compare January 6, 2025 11:08

bshephar marked this pull request as draft January 9, 2025 00:19

openshift-ci bot added the do-not-merge/work-in-progress label Jan 9, 2025

bshephar force-pushed the refactor-cleanup branch 2 times, most recently from 3d38314 to 5c88425 Compare February 6, 2025 22:32

bshephar marked this pull request as ready for review February 6, 2025 23:38

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 6, 2025

openshift-ci bot requested review from abays and dprince February 6, 2025 23:38

bshephar marked this pull request as draft February 6, 2025 23:38

openshift-ci bot added the do-not-merge/work-in-progress label Feb 6, 2025

openshift-merge-robot added the needs-rebase label Feb 7, 2025

bshephar force-pushed the refactor-cleanup branch 2 times, most recently from b705b6a to fa0ce3c Compare February 7, 2025 00:17

openshift-merge-robot removed the needs-rebase label Feb 7, 2025

bshephar force-pushed the refactor-cleanup branch 2 times, most recently from 8b021a6 to 6c84979 Compare February 10, 2025 11:57

openshift-merge-robot added the needs-rebase label Feb 10, 2025

bshephar force-pushed the refactor-cleanup branch from 6c84979 to adb11d0 Compare February 11, 2025 04:11

openshift-merge-robot removed the needs-rebase label Feb 11, 2025

bshephar commented Feb 12, 2025

View reviewed changes

pkg/horizon/deployment.go Outdated Show resolved Hide resolved

bshephar force-pushed the refactor-cleanup branch from adb11d0 to 4b0b057 Compare February 13, 2025 06:10

bshephar marked this pull request as ready for review February 15, 2025 23:27

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 15, 2025

bshephar added 2 commits February 16, 2025 09:29

Add tests for TLS mounts and volumes

9a56517

Signed-off-by: Brendan Shephard <[email protected]>

bshephar force-pushed the refactor-cleanup branch from dc2703a to 8c236c8 Compare February 15, 2025 23:30

Add Probes to envtest checks

7d2cef9

Signed-off-by: Brendan Shephard <[email protected]>

bshephar force-pushed the refactor-cleanup branch from 8c236c8 to 7d2cef9 Compare February 15, 2025 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor deployment.go code #365

Refactor deployment.go code #365

bshephar commented Sep 19, 2024 •

edited

Loading

openshift-ci bot commented Sep 19, 2024

bshephar commented Sep 21, 2024

bshephar commented Jan 9, 2025

bshephar commented Feb 6, 2025

bshephar commented Feb 7, 2025

bshephar commented Feb 7, 2025

bshephar commented Feb 7, 2025 •

edited

Loading

stuggi commented Feb 7, 2025

bshephar commented Feb 10, 2025

bshephar commented Feb 11, 2025

bshephar commented Feb 13, 2025

bshephar commented Feb 13, 2025

bshephar commented Feb 14, 2025

Refactor deployment.go code #365

Are you sure you want to change the base?

Refactor deployment.go code #365

Conversation

bshephar commented Sep 19, 2024 • edited Loading

openshift-ci bot commented Sep 19, 2024

bshephar commented Sep 21, 2024

bshephar commented Jan 9, 2025

bshephar commented Feb 6, 2025

bshephar commented Feb 7, 2025

bshephar commented Feb 7, 2025

bshephar commented Feb 7, 2025 • edited Loading

stuggi commented Feb 7, 2025

bshephar commented Feb 10, 2025

bshephar commented Feb 11, 2025

bshephar commented Feb 13, 2025

bshephar commented Feb 13, 2025

bshephar commented Feb 14, 2025

bshephar commented Sep 19, 2024 •

edited

Loading

bshephar commented Feb 7, 2025 •

edited

Loading