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.

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

MGDAPI-5640 - allow 0 value maintenance hour #680

Merged
merged 2 commits into from
Apr 17, 2023

Conversation

adam-cattermole
Copy link
Member

@adam-cattermole adam-cattermole commented Apr 14, 2023

Overview

MGDAPI-5640

What

Due to the configuration of the maintenanceWindow, 0 values are omitted during serialization as they are marked omit empty. I have included our version of the MaintenanceWindow in gcpiface with the Day and Hour parameters set as pointers. This allows us to determine if the parameter is set or not (nil = omitted), and also whether it is set but a zero value. In the case of days the permitted values are only 1-7 and so we do not set ForceSendFields (as an omitted value is not valid). However in the case of hours we set the ForceSendFields in the case of zero values to ensure that the value is serialized before GCP API calls.

Also updated code in the updateStrategy to monitor changes to the maintenance and reflect them in GCP.

Verification

It would be easiest to reproduce alongside the integreatly-operator where the issue first arose.

Provision a GCP CCS cluster

From the master branch of the Delorean repo create a file in the root of the repo called gcp_service_account.json. Request GCP credentials and add these to this file. Adjust this command to suit your needs, it will generate a config at ocm/cluster.json:

OCM_CLUSTER_NAME=acatterm-ccs OCM_CLUSTER_LIFESPAN=24 COMPUTE_NODES_COUNT=4 BYOC=true CLOUD_PROVIDER=gcp make -f make/ocm.mk ocm/cluster.json

Once you have checked the ocm/cluster.json, create the cluster:

make -f make/ocm.mk ocm/cluster/create

Verify changes

Install RHOAM from master with MaintenanceWindow set as 0,0:

MAINTENANCE_DAY=0 MAINTENANCE_HOUR=0 LOCAL=false make cluster/prepare/local
LOCAL=false USE_CLUSTER_STORAGE=false make code/run

Installation should get stuck creating the initial Postgres instance due to the hour field not being set in the strategy:

oc get configmap cloud-resources-gcp-strategies -n redhat-rhoam-operator -o yaml | yq '.data.postgres' | jq '.production.createStrategy'

{
  "instance": {
    "settings": {
      "backupConfiguration": {
        "startTime": "03:01"
      },
      "maintenanceWindow": {
        "day": 7
      }
    }
  }
}

Stop the operator and checkout the changes from my branch https://github.com/adam-cattermole/integreatly-operator/tree/MGDAPI-5640 (this is consuming API changes + new CRO v1.0.2 from this PR), rerun the operator.

The ConfigMap should update to include the hour:

oc get configmap cloud-resources-gcp-strategies -n redhat-rhoam-operator -o yaml | yq '.data.postgres' | jq '.production.createStrategy'

{
  "instance": {
    "settings": {
      "backupConfiguration": {
        "startTime": "03:01"
      },
      "maintenanceWindow": {
        "day": 7,
        "hour": 0
      }
    }
  }
}

The Postgres creation will still fail until CRO updates to v1.0.2 (dev version including these changes) - wait for this to take place.

RHOAM installation should complete as expected.

@adam-cattermole adam-cattermole changed the title [WIP] MGDAPI-5640 - allow 0 value maintenance hour MGDAPI-5640 - allow 0 value maintenance hour Apr 14, 2023
@codecov
Copy link

codecov bot commented Apr 14, 2023

Codecov Report

Merging #680 (4dfbbe0) into master (0d21286) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #680      +/-   ##
==========================================
+ Coverage   72.20%   72.24%   +0.03%     
==========================================
  Files          53       53              
  Lines        6765     6774       +9     
==========================================
+ Hits         4885     4894       +9     
  Misses       1487     1487              
  Partials      393      393              
Impacted Files Coverage Δ
pkg/client/gcp/strategies_gcp.go 71.42% <100.00%> (ø)
pkg/providers/gcp/provider_postgres.go 79.85% <100.00%> (+0.43%) ⬆️

@adam-cattermole
Copy link
Member Author

/test coverage

@MStokluska
Copy link
Member

I've confirmed the logic working with RHOAM. Looks good to me.
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Apr 17, 2023
@austincunningham
Copy link
Member

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: austincunningham

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 3eca0a2 into integr8ly:master Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants