external dns: release 1.1 #34949
external dns: release 1.1 #34949
Conversation
|
@alebedev87: the following rehearsable tests have been affected by this change:
Prior to this PR being merged, you will need to either run and acknowledge or opt to skip these rehearsals. Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
openshift-ci
bot
added
the
approved
alebedev87
force-pushed
the
ext-dns-release-1-1
branch
from
7cf3982 to
9b0fdad
Compare
|
/pj-rehearse |
alebedev87
force-pushed
the
ext-dns-release-1-1
branch
from
9b0fdad to
d094210
Compare
|
/pj-rehearse |
alebedev87
force-pushed
the
ext-dns-release-1-1
branch
from
d094210 to
26e629a
Compare
|
/pj-rehearse |
|
/pj-rehearse ack |
openshift-ci-robot
added
the
rehearsals-ack
alebedev87
force-pushed
the
ext-dns-release-1-1
branch
from
26e629a to
252eb1e
Compare
|
Promotion section: |
|
/assign @rfredette |
|
@alebedev87 @Miciah since these files contain references to Openshift versions https://github.com/openshift/release/pull/34949/files#diff-c2cb2f4140a4cf2ff73fc4527659ce4d0d45bb9f3887e8041e2a9b06ffc1cf21R10, I wonder if we need to label files with the openshift image number, as we do for most of our CI config/jobs, like https://github.com/openshift/release/tree/master/ci-operator/config/openshift/cluster-ingress-operator, and even for things like https://github.com/openshift/release/tree/master/ci-operator/config/openshift/azure-disk-csi-driver-operator, which may be handled more like external-dns and not tied to specific Openshift versions. These files are generated automatically for each release so it's not a effort disadvantage. |
|
@candita : I believe we should do the same as maistra - keep only the external dns operator versioned configs. We don't use the OCP versioned branches for extrernal dns operator. |
@alebedev87 I noticed that about ALBO, but you were the author of that one as well :). So, I guess I will be asking some more about why Openshift versions are mentioned in the yaml for external dns if we don't use the versioned branches. |
| @@ -0,0 +1,152 @@ | |||
| base_images: | |||
| base: | |||
| name: "4.12" | |||
There was a problem hiding this comment.
ince external-dns isn't tied to an OpenShift release, it is confusing to use or reference OpenShift or OpenShift versions. Can you use a different name, namespace, tag, builder etc? I expect to not see "4.12", "ocp", or "openshift" anywhere in here.
There was a problem hiding this comment.
I don't see a big problem in using the OCP image here. It'll be used as the base (Docker FROM) image for the operator to run the tests. I think of it as a sort of minimal Kube version in combination with RHEL which covers the most of the future customers of this operator.
There was a problem hiding this comment.
That sounds reasonable. Could you please get someone from the release team to validate that you have what is needed here, in order to generate updates for releases after 4.12? If there are OpenShift CI updates after 4.12, this config should be using them, and the things I suggested so far that I know about, you have not agreed would work. So probably we need an expert on the generation of release CI files.
There was a problem hiding this comment.
Found ubi-minimal imagestream. Should be enough for the external dns repositories.
| workflow: optional-operators-ci-azure | ||
| - as: e2e-infoblox-operator | ||
| steps: | ||
| cluster_profile: azure4 |
There was a problem hiding this comment.
infoblox uses the same cluster_profile as azure?
There was a problem hiding this comment.
Yes. It doesn't really matter which cloud provider the Infoblox test case will be run at. Since we don't request the credentials from the CloudCredentialsOperator of the cluster like for AWS, GCP and Azure. So, the choice of the cluster profile is a pure convenience for Infoblox case - we chose Azure recently due to a lot of issues on GCP and AWS CI accounts.
There was a problem hiding this comment.
I noticed the failure of e2e-azure-ovn-infoblox-operator test. Just wondering if something needs to change here.
There was a problem hiding this comment.
No, the temporary Infoblox licenses expired, they last 3 months. We still use the temporary licenses for the Infoblox product from AWS marketplace as we didn't manage to get any from Infoblox (company). We'll have to re-iterate with DPTP on this.
Reset the instance, reran the rehearsal.
| workflow: optional-operators-ci-azure | ||
| zz_generated_metadata: | ||
| branch: release-1.1 | ||
| org: openshift |
There was a problem hiding this comment.
org should not be openshift.
There was a problem hiding this comment.
I don't think we have a choice here. Since we use the Openshift CI for external-dns-operator (like a lot of other addon operators do). This is a metadata generated by make ci-operator-config, without it the PR wouldn't pass the validation.
There was a problem hiding this comment.
yes, this is correct here (it's the openshift in github.com/openshift/external-dns-operator/tree/release-1.1)
| name: "4.12" | ||
| namespace: ocp | ||
| tag: base | ||
| ocp_builder_rhel-8-golang-1.18-openshift-4.12: |
There was a problem hiding this comment.
golang-1.19 is used for the external-dns-operator, and 1.18 for this? It would be better to use the same golang version or a comment on the difference.
There was a problem hiding this comment.
Yes, it would be better. We have less control over the controller code as it's rebased from the upstream. For this release the upstream wasn't updated to 1.19 yet.
| - agent: kubernetes | ||
| always_run: true | ||
| branches: | ||
| - ^release-1\.1$ |
There was a problem hiding this comment.
Shouldn't the presubmit happen off the master branch, like https://github.com/openshift/release/blob/master/ci-operator/jobs/openshift/cluster-ingress-operator/openshift-cluster-ingress-operator-master-presubmits.yaml#L6 ?
There was a problem hiding this comment.
Why should it happen off the master branch for release-1.1 branch?
Btw this file was generated by make jobs off the config I added, so I guess it's the normal setup for a new branch.
There was a problem hiding this comment.
I think the release-. code is generated using the *-master-presubmits.yaml, or maybe openshift-external-dns-operator-main.yaml in this case.
There was a problem hiding this comment.
Something is off - take a look at the structure in https://github.com/openshift/release/tree/master/ci-operator/config/openshift/external-dns-operator
There was a problem hiding this comment.
Sorry, I'm not getting the point. To me the presubmits which target the branch release-1.1 should happen off release-1.1 branch. The release before the previous release (1.0 is missing the CI config btw) was doing the same. The cluster-ingress-operator's 4.11 release branch presubmit uses release-4.11 branches too.
There was a problem hiding this comment.
Was this file generated or created manually?
There was a problem hiding this comment.
Generated, by make jobs from the config I put in this PR. I believe it's just the naming convention of the config file which contains the branch name.
There was a problem hiding this comment.
This looks all good, but there should be another config testing the main branch (there's no master in https://github.com/openshift/external-dns-operator/)
| make test | ||
| container: | ||
| from: src | ||
| - as: e2e-aws-operator |
There was a problem hiding this comment.
Is external-dns supported on ovn?
There was a problem hiding this comment.
You mean the network plugin? OVN is now the default network plugin of OpenShift (couldn't find from which version though). So, I suppose the answer is yes. Also because I cannot remember anything specific to a particular network plugin we used in the operator or controller.
There was a problem hiding this comment.
You should add the "ovn" to the test name where needed to make sure this runs the OVN tests without flakes. See #31430 - it's a change needed in multiple places.
alebedev87
force-pushed
the
ext-dns-release-1-1
branch
from
252eb1e to
53b6757
Compare
|
@candita : thanks for the review!
As a matter of fact it was Arjun but anyway, I agree with your point about the OCP files in the external-dns configs. I will follow up on this one to exclude the external dns repositories from the OCP job (there seems to be an automation around the branching for OCP dependent repositories). Also, I cleaned up the configs to remove all unnecessary OCP references (builder images, kube-rbac-proxy). However some references are still fine (OCP base image) or needed (Dockerfile.openshift for external-dns repository). |
|
/pj-rehearse |
alebedev87
force-pushed
the
ext-dns-release-1-1
branch
from
53b6757 to
a2577f7
Compare
|
/unlabel rehearsals-ack |
|
/pj-rehearse |
|
/remove-label rehearsals-ack |
|
@alebedev87: The label(s) In response to this:
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. |
|
/pj-rehearse reject |
openshift-ci-robot
removed
the
rehearsals-ack
|
/pj-rehearse |
1 similar comment
|
/pj-rehearse |
|
/pj-rehearse ack |
openshift-ci-robot
added
the
rehearsals-ack
|
@alebedev87 overall, since this product is like maistra, I don't know why you need to specify OCP 4.12 in some of the configuration fields. I suggest getting a review from someone on the CI team, just to make sure you're not setting a precedent here that will be hard to change later. So, I mark with lgtm and a hold, which can be removed after the review from the CI team. If you feel that a review from the CI team isn't necessary, feel free to remove the hold yourself. /lgtm |
openshift-ci
bot
added
do-not-merge/hold
|
@alebedev87 Also, I took a look at the current state of these directories, and I think if in this PR you remove the |
|
/cc @LorbusChris - would you be able to take a look? |
|
Finally, just wanted to know if there was ever a release 1.0? Or a reason why the version moved from 0.1 to 1.1? |
Yes, there was. The CI for it was not implemented, I mentioned this in another thread before. I'll do a separate PR for the release 1.0. |
As I briefly mentioned before, I will follow up on this and will push a separate PR to remove all OCP related branch configs. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87, candita, LorbusChris 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 |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@alebedev87: 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. |
|
@alebedev87: Updated the following 3 configmaps:
In response to this:
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. |
The CI configuration for the release branch
1.1of ExternalDNS and ExternalDNS Operator.The mirroring for
1.1release is made toquay.io/external-dns-operator/external-dns:1.1(notlatest).