Skip to content

Conversation

@mprahl
Copy link
Member

@mprahl mprahl commented Feb 10, 2023

When a ManifestWork without UpdateStrategy set is marshalled into JSON, it causes the JSON value of "updateStrategy": null. This works on newer versions of Kubernetes, however, on older versions such as v1.19.16, the following error is returned when creating a ManifestWork: The ManifestWork "addon-config-policy-controller-pre-delete-hosting-cluster2" is invalid: spec.manifestConfigs.updateStrategy: Invalid value: "null": spec.manifestConfigs.updateStrategy in body must be of type object: "null"

The best way to avoid this is to add omitempty to the JSON tag so that it's completely not present in the JSON representation.

This bug was encountered when using a predelete hook in the addon framework. So this change will need to be updated in at least the addon framework and the work agent.

Relates:
https://issues.redhat.com/browse/ACM-3233
https://issues.redhat.com/browse/ACM-2923

Signed-off-by: mprahl mprahl@users.noreply.github.com

@openshift-ci openshift-ci bot requested review from deads2k and mdelder February 10, 2023 15:21
@mprahl
Copy link
Member Author

mprahl commented Feb 10, 2023

/cc @qiujian16

When a ManifestWork without UpdateStrategy set is marshalled into JSON, it
causes the JSON value of "updateStrategy": null. This works on newer
versions of Kubernetes, however, on older versions such as v1.19.16, the
following error is returned when creating a ManifestWork:
The ManifestWork "addon-config-policy-controller-pre-delete-hosting-cluster2" is invalid: spec.manifestConfigs.updateStrategy: Invalid value: "null": spec.manifestConfigs.updateStrategy in body must be of type object: "null"

The best way to avoid this is to add omitempty to the JSON tag so that
it's completely not present in the JSON representation.

This bug was encountered when using a predelete hook in the addon
framework. So this change will need to be updated in at least the addon
framework and the work agent.

Relates:
https://issues.redhat.com/browse/ACM-3233
https://issues.redhat.com/browse/ACM-2923

Signed-off-by: mprahl <mprahl@users.noreply.github.com>
@mprahl mprahl requested review from JustinKuli and removed request for deads2k, mdelder and qiujian16 February 10, 2023 20:03
@qiujian16
Copy link
Member

/approve
/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mprahl, qiujian16

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 b33259f into open-cluster-management-io:main Feb 13, 2023
openshift-merge-robot pushed a commit to open-cluster-management-io/governance-policy-addon-controller that referenced this pull request Feb 13, 2023
This change makes it so that a finalizer is added to the
config-policy-controller ManagedClusterAddOn object. Now when it is deleted,
it will trigger an additional ManifestWork to create a Pod which
signals to the config-policy-controller that it's being uninstalled and
it waits for all ConfigurationPolicy objects to have their finalizers
removed before exiting. Once the Pod exits successfully, the finalizer
on the ManagedClusterAddOn object is removed and the
config-policy-controller uninstall proceeds.

If there are a lot of policies, it may take longer than 30 seconds, so
the grace period was bumped up to 120 seconds to exit cleanly.

Note that now with KIND_VERSION set to minimum, it uses a newer version
for the Hub to match the lowest supported version for the Hub. This is
to work around this:
open-cluster-management-io/api#206

Signed-off-by: mprahl <mprahl@users.noreply.github.com>
magic-mirror-bot bot pushed a commit to stolostron/governance-policy-addon-controller that referenced this pull request Feb 13, 2023
This change makes it so that a finalizer is added to the
config-policy-controller ManagedClusterAddOn object. Now when it is deleted,
it will trigger an additional ManifestWork to create a Pod which
signals to the config-policy-controller that it's being uninstalled and
it waits for all ConfigurationPolicy objects to have their finalizers
removed before exiting. Once the Pod exits successfully, the finalizer
on the ManagedClusterAddOn object is removed and the
config-policy-controller uninstall proceeds.

If there are a lot of policies, it may take longer than 30 seconds, so
the grace period was bumped up to 120 seconds to exit cleanly.

Note that now with KIND_VERSION set to minimum, it uses a newer version
for the Hub to match the lowest supported version for the Hub. This is
to work around this:
open-cluster-management-io/api#206

Signed-off-by: mprahl <mprahl@users.noreply.github.com>
(cherry picked from commit b82633184bf18fdfe1705440c30f8a1672c46702)
openshift-merge-robot pushed a commit to stolostron/governance-policy-addon-controller that referenced this pull request Feb 14, 2023
This change makes it so that a finalizer is added to the
config-policy-controller ManagedClusterAddOn object. Now when it is deleted,
it will trigger an additional ManifestWork to create a Pod which
signals to the config-policy-controller that it's being uninstalled and
it waits for all ConfigurationPolicy objects to have their finalizers
removed before exiting. Once the Pod exits successfully, the finalizer
on the ManagedClusterAddOn object is removed and the
config-policy-controller uninstall proceeds.

If there are a lot of policies, it may take longer than 30 seconds, so
the grace period was bumped up to 120 seconds to exit cleanly.

Note that now with KIND_VERSION set to minimum, it uses a newer version
for the Hub to match the lowest supported version for the Hub. This is
to work around this:
open-cluster-management-io/api#206

Signed-off-by: mprahl <mprahl@users.noreply.github.com>
(cherry picked from commit b82633184bf18fdfe1705440c30f8a1672c46702)
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/governance-policy-addon-controller that referenced this pull request Feb 14, 2023
This change makes it so that a finalizer is added to the
config-policy-controller ManagedClusterAddOn object. Now when it is deleted,
it will trigger an additional ManifestWork to create a Pod which
signals to the config-policy-controller that it's being uninstalled and
it waits for all ConfigurationPolicy objects to have their finalizers
removed before exiting. Once the Pod exits successfully, the finalizer
on the ManagedClusterAddOn object is removed and the
config-policy-controller uninstall proceeds.

If there are a lot of policies, it may take longer than 30 seconds, so
the grace period was bumped up to 120 seconds to exit cleanly.

Note that now with KIND_VERSION set to minimum, it uses a newer version
for the Hub to match the lowest supported version for the Hub. This is
to work around this:
open-cluster-management-io/api#206

Signed-off-by: mprahl <mprahl@users.noreply.github.com>
(cherry picked from commit b82633184bf18fdfe1705440c30f8a1672c46702)
mprahl added a commit to stolostron/governance-policy-addon-controller that referenced this pull request Feb 14, 2023
This change makes it so that a finalizer is added to the
config-policy-controller ManagedClusterAddOn object. Now when it is deleted,
it will trigger an additional ManifestWork to create a Pod which
signals to the config-policy-controller that it's being uninstalled and
it waits for all ConfigurationPolicy objects to have their finalizers
removed before exiting. Once the Pod exits successfully, the finalizer
on the ManagedClusterAddOn object is removed and the
config-policy-controller uninstall proceeds.

If there are a lot of policies, it may take longer than 30 seconds, so
the grace period was bumped up to 120 seconds to exit cleanly.

Note that now with KIND_VERSION set to minimum, it uses a newer version
for the Hub to match the lowest supported version for the Hub. This is
to work around this:
open-cluster-management-io/api#206

Signed-off-by: mprahl <mprahl@users.noreply.github.com>
(cherry picked from commit b82633184bf18fdfe1705440c30f8a1672c46702)
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