OCPBUGS-29934: Add support to inject owner-ref argument to render command #960

rbaturov · 2024-02-19T13:29:11Z

We need this change because we also want to use also non default values for the owner-ref like the value none.
The work on this PR depends on these modifications to be merged.

rbaturov · 2024-02-19T13:37:32Z

@Tal-or @ffromani

ffromani · 2024-02-19T14:06:52Z

this works, but IMO it should work the other way around: the owner-ref should be on by default, and opted-out only in the few (currently, exactly one and only) cases which don't need it

MarSik · 2024-02-19T14:14:20Z

hack/render-sync.sh

I think mixing directories and ownership reference arguments is confusing a bit. The traditional unix way would be something like:

rendersync --owner-ref label-name -- manual-cluster/performance base/performance default

In other words, extra arguments followed by -- separator and then the rendering arguments.

The code to implement this is a bit more complicated than what you have here, but I believe it would make the code less error prone.

This would be the best option indeed. I was about to settle for a little less, but if we can get this far that would be great.

The only thing that bothers me with this approach is that we basically implement render command line again but in the lesser convenient way which is using bash for parsing the arguments.

Maybe we can enhance render command to read its configuration from yaml file something like:

config: - inputDir: "foo" outputDir: "bar" ownerRef: none - inputDir: "foo2" outputDir: "bar2" ownerRef: k8s - inputDir: "foo2" outputDir: "bar2" // use default owner-ref

This way we can list all the different rendering configuration in a single file, parsing it as part of the code and avoid error-prone shell scripting

I would be fine with a stopgap solution (but with the defaults swapped per my comment above) to unblock this work and with a cleaner design later while we figure out the best tradeoff

I think we can implement my proposal like this. Just below line 7 include

EXTRA_ARGS=() if [[ "$1" =~ ^-.* ]]; then while [[ "x$1" != "x--" ]]; do EXTRA_ARGS+=("$1") shift done shift fi

And then in the execution of render use "${EXTRA_ARGS[@]}"

We should make the right value the default one in the render command so we do not have to use this method too often.

We need this change because we want to use also non default values for the owner-ref like the value none.

rbaturov · 2024-02-20T11:34:27Z

/retest-required

ffromani · 2024-02-20T12:41:52Z

LGTM
how do we test this change?

Tal-or · 2024-02-20T13:05:04Z

LGTM how do we test this change?

There was a test which was failing and exposed the issue. if the test will pass then it mean we're out of the woods

ffromani · 2024-02-20T13:25:16Z

/approve
/lgtm

per #960 (comment)

openshift-ci · 2024-02-20T13:26:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, rbaturov

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

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

ffromani · 2024-02-20T14:14:13Z

/label acknowledge-critical-fixes-only

ffromani · 2024-02-20T14:14:26Z

this PR won't affect the payload

ffromani · 2024-02-20T14:16:26Z

/retitle NO-JIRA: Add support to inject owner-ref argument to render command

openshift-ci-robot · 2024-02-20T14:17:02Z

@rbaturov: This pull request explicitly references no jira issue.

In response to this:

We need this change because we also want to use also non default values for the owner-ref like the value none.
The work on this PR depends on these modifications to be merged.

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 openshift-eng/jira-lifecycle-plugin repository.

rbaturov · 2024-02-20T16:14:28Z

/retest-required

openshift-ci · 2024-02-20T18:41:46Z

@rbaturov: all tests passed!

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2024-02-21T02:10:01Z

[ART PR BUILD NOTIFIER]

This PR has been included in build cluster-node-tuning-operator-container-v4.16.0-202402210139.p0.g631e03c.assembly.stream.el9 for distgit cluster-node-tuning-operator.
All builds following this will include this PR.

rbaturov · 2024-02-21T13:23:09Z

/cherry-pick release-4.15

openshift-cherrypick-robot · 2024-02-21T13:23:50Z

@rbaturov: new pull request created: #966

In response to this:

/cherry-pick release-4.15

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/test-infra repository.

openshift-ci-robot · 2024-02-26T14:52:05Z

@rbaturov: Jira Issue OCPBUGS-29934: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-29934 has been moved to the MODIFIED state.

In response to this:

We need this change because we also want to use also non default values for the owner-ref like the value none.
The work on this PR depends on these modifications to be merged.

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 openshift-eng/jira-lifecycle-plugin repository.

rbaturov · 2024-02-28T13:04:07Z

/cherry-pick release-4.14

openshift-cherrypick-robot · 2024-02-28T13:04:57Z

@rbaturov: #960 failed to apply on top of branch "release-4.14":

Applying: Add support to inject owner-ref argument to render command
Using index info to reconstruct a base tree...
M	hack/render-sync.sh
Falling back to patching base and 3-way merge...
Auto-merging hack/render-sync.sh
CONFLICT (content): Merge conflict in hack/render-sync.sh
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add support to inject owner-ref argument to render command
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.14

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/test-infra repository.

…#960) We need this change because we want to use also non default values for the owner-ref like the value none. (cherry picked from commit 631e03c)

…ner reference ehnancements (#989) * NO-JIRA: perfprof: render: make ownerReference optional (#907) * perfprof: render: make ownerReference optional When we set OwnerReference for dependant object, we must use the object UID the apiserver set. Apiserver owns this field (obviously) so setting it when missing, like the code did in the past, is wrong and will cause conflicts on updates. I the pre-render flow we can't wait for the apiserver to set a UID, so the only possible option is to not set any OwnerReference at all. Signed-off-by: Francesco Romani <fromani@redhat.com> * perfprof: render: add weak owner reference support There are cases on which we can't use the real owner reference in the render flow, because: - we don't own the performanceprofile uid - we can't wait to learn back the apiserver-decided uid because we need to render all the manifest at once. so, to group objects together and enable to safely and easily rebuild the ownerReference graph later, we add an annotation which serves as weaker owner reference, using the profile name and adding it to all the related objects. Note this form of weak reference is meant to serve to a placeholder for the real ownerReference which should be added by the relevant controller later on, when the cluster is started and is running. Signed-off-by: Francesco Romani <fromani@redhat.com> --------- Signed-off-by: Francesco Romani <fromani@redhat.com> (cherry picked from commit 0e31943) (cherry picked from commit 5308f47) * release-4.14: render: align expected machineconfig the backported expected machineconfig output was including the changes in 4.15, we need to update with the 4.14-specific content, excluding unrelated changes. Signed-off-by: Francesco Romani <fromani@redhat.com> * render: perfprofile: don't annotate perfprof (#951) don't add weak ref to the perfprof and don't write back an updated version in output. Signed-off-by: Francesco Romani <fromani@redhat.com> (cherry picked from commit fd8ea4e) (cherry picked from commit 34926bd) * e2e: testdata: remove the annotated profile after #951 merged, we don't need these anymore Signed-off-by: Francesco Romani <fromani@redhat.com> (cherry picked from commit 0d157b5) * CNF-11145: Enhance render sync to include bootstrap rendering tests (#932) * Enhance render sync to include bootstrap rendering tests * make render-sync (cherry picked from commit e225a86) * Add support to inject owner-ref argument to render command (#960) We need this change because we want to use also non default values for the owner-ref like the value none. (cherry picked from commit 631e03c) * release-4.14: hack: branch-specific updates remove lines not relevant for this branch Signed-off-by: Francesco Romani <fromani@redhat.com> --------- Signed-off-by: Francesco Romani <fromani@redhat.com> Co-authored-by: Martin Sivák <msivak@redhat.com> Co-authored-by: Ronny Baturov <rbaturov@redhat.com>

rbaturov force-pushed the add-owner-ref-arg-support branch from edc4976 to 3b6ebd1 Compare February 19, 2024 13:32

openshift-ci bot requested review from dagrayvid and swatisehgal February 19, 2024 13:33

MarSik reviewed Feb 19, 2024

View reviewed changes

Add support to inject owner-ref argument to render command

b4d7df3

We need this change because we want to use also non default values for the owner-ref like the value none.

rbaturov force-pushed the add-owner-ref-arg-support branch from 3b6ebd1 to b4d7df3 Compare February 19, 2024 18:02

rbaturov mentioned this pull request Feb 20, 2024

OCPBUGS-23167: Add performance real time tuned template #954

Merged

openshift-ci bot assigned ffromani Feb 20, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2024

openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Feb 20, 2024

openshift-ci bot changed the title ~~Add support to inject owner-ref argument to render command~~ NO-JIRA: Add support to inject owner-ref argument to render command Feb 20, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 20, 2024

openshift-merge-bot bot merged commit 631e03c into openshift:master Feb 20, 2024
15 checks passed

rbaturov deleted the add-owner-ref-arg-support branch February 20, 2024 18:51

openshift-cherrypick-robot mentioned this pull request Feb 21, 2024

[release-4.15] OCPBUGS-29936: Add support to inject owner-ref argument to render command #966

Merged

rbaturov changed the title ~~NO-JIRA: Add support to inject owner-ref argument to render command~~ 29934: Add support to inject owner-ref argument to render command Feb 26, 2024

rbaturov changed the title ~~29934: Add support to inject owner-ref argument to render command~~ OCPBUGS-29934: Add support to inject owner-ref argument to render command Feb 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-29934: Add support to inject owner-ref argument to render command #960

OCPBUGS-29934: Add support to inject owner-ref argument to render command #960

rbaturov commented Feb 19, 2024

rbaturov commented Feb 19, 2024

ffromani commented Feb 19, 2024 •

edited

Loading

MarSik Feb 19, 2024

ffromani Feb 19, 2024

Tal-or Feb 19, 2024

Tal-or Feb 19, 2024 •

edited

Loading

ffromani Feb 19, 2024

MarSik Feb 19, 2024 •

edited

Loading

rbaturov commented Feb 20, 2024

ffromani commented Feb 20, 2024

Tal-or commented Feb 20, 2024

ffromani commented Feb 20, 2024

openshift-ci bot commented Feb 20, 2024

ffromani commented Feb 20, 2024

ffromani commented Feb 20, 2024

ffromani commented Feb 20, 2024

openshift-ci-robot commented Feb 20, 2024

rbaturov commented Feb 20, 2024

openshift-ci bot commented Feb 20, 2024

openshift-bot commented Feb 21, 2024

rbaturov commented Feb 21, 2024

openshift-cherrypick-robot commented Feb 21, 2024

openshift-ci-robot commented Feb 26, 2024

rbaturov commented Feb 28, 2024

openshift-cherrypick-robot commented Feb 28, 2024

OCPBUGS-29934: Add support to inject owner-ref argument to render command #960

OCPBUGS-29934: Add support to inject owner-ref argument to render command #960

Conversation

rbaturov commented Feb 19, 2024

rbaturov commented Feb 19, 2024

ffromani commented Feb 19, 2024 • edited Loading

MarSik Feb 19, 2024

Choose a reason for hiding this comment

ffromani Feb 19, 2024

Choose a reason for hiding this comment

Tal-or Feb 19, 2024

Choose a reason for hiding this comment

Tal-or Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

ffromani Feb 19, 2024

Choose a reason for hiding this comment

MarSik Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

rbaturov commented Feb 20, 2024

ffromani commented Feb 20, 2024

Tal-or commented Feb 20, 2024

ffromani commented Feb 20, 2024

openshift-ci bot commented Feb 20, 2024

ffromani commented Feb 20, 2024

ffromani commented Feb 20, 2024

ffromani commented Feb 20, 2024

openshift-ci-robot commented Feb 20, 2024

rbaturov commented Feb 20, 2024

openshift-ci bot commented Feb 20, 2024

openshift-bot commented Feb 21, 2024

rbaturov commented Feb 21, 2024

openshift-cherrypick-robot commented Feb 21, 2024

openshift-ci-robot commented Feb 26, 2024

rbaturov commented Feb 28, 2024

openshift-cherrypick-robot commented Feb 28, 2024

ffromani commented Feb 19, 2024 •

edited

Loading

Tal-or Feb 19, 2024 •

edited

Loading

MarSik Feb 19, 2024 •

edited

Loading