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

whereabouts: bump to v0.5.4 #847

Merged
merged 1 commit into from
Aug 11, 2022
Merged

Conversation

jean-edouard
Copy link
Contributor

@jean-edouard jean-edouard commented Jul 29, 2022

We're currently using v0.5, which appears to be unreliable, probably due to the race condition issues they've been working on upstream.
v0.5.4 should contain all the necessary fixes.

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Jul 29, 2022
@jean-edouard
Copy link
Contributor Author

Added a commit to switch to Fedora 36 for building the Centos 8 image (oO).
The static hash there doesn't seem to work anymore.
Probably not the right thing to do but otherwise check-provision fails right away and I want to test this PR.

Copy link
Contributor

@oshoval oshoval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, nice
Can you please update PR desc about the reason as appears here?
kubevirt/kubevirt#7168 (comment)

Also if you can update PR desc about pinning to fedora:36 because the hash doesn't work

@@ -6,6 +6,3 @@ resources:
- ./daemonset-install.yaml
- ./whereabouts.cni.cncf.io_ippools.yaml
- ./whereabouts.cni.cncf.io_overlappingrangeipreservations.yaml

transformers:
Copy link
Contributor

@oshoval oshoval Jul 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the files appear above are deleted as well ?
(they were merged into the current file)
also once there isn't a transformers we basically not need this file and the references of it

On the other hand, we will need on a follow PR to use kustomize again in order to pin the version
until k8snetworkplumbingwg/whereabouts#252 is fixed
(unless it is already fixed for 0.5.4)

@jean-edouard jean-edouard marked this pull request as draft August 1, 2022 15:17
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2022
@jean-edouard jean-edouard marked this pull request as ready for review August 1, 2022 17:46
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 1, 2022
@jean-edouard
Copy link
Contributor Author

Removed the fedora fix commit, as it was addressed by another (already merged) PR.
Added the appropriate kustomize bits to address the image issues upstream.
PTAL!

@@ -7,5 +7,16 @@ resources:
- ./whereabouts.cni.cncf.io_ippools.yaml
- ./whereabouts.cni.cncf.io_overlappingrangeipreservations.yaml

transformers:
- patch-ip-reconciler-job.yaml
patches:
Copy link
Contributor

@oshoval oshoval Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes
Can it be done please with this kustomization style ?
https://github.com/kubevirt/kubevirtci/blob/main/cluster-up/cluster/kind-1.22-sriov/sriov-components/manifests/kustomization.yaml#L9

and then the sed in the script is not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you pass the version number to kustomize without using sed?

Copy link
Contributor

@oshoval oshoval Aug 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newTag i think ? (just have the version hardcoded in it)

something like this (newName is not needed)

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- grafana-deployment.yaml
images:
  - name: grafana/grafana
    newName: quay.io/kubevirtci/grafana-grafana
    newTag: <TAG>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And.. how do you set <TAG>? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

envsubst / sed it according "whereabouts_version", wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, PTAL!

@oshoval
Copy link
Contributor

oshoval commented Aug 2, 2022

Please also update the manifest of k8s-1.24-ipv6

and if you can update please the PR desc with the reasoning as you found here
kubevirt/kubevirt#7168 (comment)
would be awesome, thanks

@oshoval
Copy link
Contributor

oshoval commented Aug 2, 2022

Hi @maiqueb
ain't 0.5.4 should pin the version as 0.5.3 does ?
k8snetworkplumbingwg/whereabouts#253
(maybe this PR was reverted ?)

Then we won't need to kustomize it ourself

@jean-edouard jean-edouard force-pushed the whereabouts54 branch 2 times, most recently from 1df643a to 4965825 Compare August 2, 2022 14:55
@maiqueb
Copy link
Contributor

maiqueb commented Aug 2, 2022

Hi @maiqueb ain't 0.5.4 should pin the version as 0.5.3 does ? k8snetworkplumbingwg/whereabouts#253 (maybe this PR was reverted ?)

Then we won't need to kustomize it ourself

v0.5.4 is using a pinned version.

... might be a bit confusing to use the generated version for v0.5.3, but, it is pinned.

Going forward, we'll try to make sure we don't screw up again.

@oshoval
Copy link
Contributor

oshoval commented Aug 2, 2022

Hi @maiqueb ain't 0.5.4 should pin the version as 0.5.3 does ? k8snetworkplumbingwg/whereabouts#253 (maybe this PR was reverted ?)
Then we won't need to kustomize it ourself

v0.5.4 is using a pinned version.

... might be a bit confusing to use the generated version for v0.5.3, but, it is pinned.

Going forward, we'll try to make sure we don't screw up again.

I am lost a bit tbh
here Jed had to pin to 0.5.4

https://github.com/kubevirt/kubevirtci/pull/847/files#diff-a6d30590f227704f4eb26e7638a13dcece7afba0a44baa1f15942ed5a47ddd50R218

@maiqueb
Copy link
Contributor

maiqueb commented Aug 2, 2022

Hi @maiqueb ain't 0.5.4 should pin the version as 0.5.3 does ? k8snetworkplumbingwg/whereabouts#253 (maybe this PR was reverted ?)
Then we won't need to kustomize it ourself

v0.5.4 is using a pinned version.
... might be a bit confusing to use the generated version for v0.5.3, but, it is pinned.
Going forward, we'll try to make sure we don't screw up again.

I am lost a bit tbh here Jed had to pin to 0.5.4

I don't follow what is it you don't understand; you asked me to pin whereabouts v0.5.4, and I've replied: it is already pinned - check the commit to which the whereabouts repo v0.5.4 release is pointing to.

https://github.com/kubevirt/kubevirtci/pull/847/files#diff-a6d30590f227704f4eb26e7638a13dcece7afba0a44baa1f15942ed5a47ddd50R218

Signed-off-by: Jed Lejosne <[email protected]>
Copy link
Contributor

@oshoval oshoval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks
Looks great

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2022
@oshoval
Copy link
Contributor

oshoval commented Aug 3, 2022

I don't follow what is it you don't understand; you asked me to pin whereabouts v0.5.4, and I've replied: it is already pinned - check the commit to which the whereabouts repo v0.5.4 release is pointing to.

I expect that users of the repo should not have to pin a version like this PR is doing
so if someone uses manifest of 0.5.4 he will have version 0.5.4 already in the image field
we can continue this conv offline if needed

Copy link
Member

@xpivarc xpivarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@enp0s3
Copy link
Contributor

enp0s3 commented Aug 10, 2022

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enp0s3

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2022
@kubevirt-commenter-bot
Copy link

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@oshoval
Copy link
Contributor

oshoval commented Aug 10, 2022

/hold

need to cordon PR to be merged first, so lanes will pass

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 10, 2022
@brianmcarey
Copy link
Member

/test check-provision-k8s-1.23

@oshoval
Copy link
Contributor

oshoval commented Aug 11, 2022

I think we are good
for example this passed today
https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_kubevirtci/827/check-provision-k8s-1.22/1557539963703136256
the PR was merged and it fixed it

we can remove the hold imo

@brianmcarey
Copy link
Member

/unhold

/retest

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 11, 2022
@kubevirt-bot kubevirt-bot merged commit 3b5306b into kubevirt:main Aug 11, 2022
kubevirt-bot added a commit to kubevirt-bot/kubevirt that referenced this pull request Aug 11, 2022
[3b5306b whereabouts: bump to v0.5.4](kubevirt/kubevirtci#847)

```release-note
NONE
```

Signed-off-by: kubevirt-bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants