search

cloudfoundry / cf-for-k8s

The open source deployment manifest for Cloud Foundry on Kubernetes
Apache License 2.0
300 stars 115 forks source link

reformat version in upgrade job #516

Closed KauzClay closed 4 years ago

KauzClay commented 4 years ago

Co-authored-by: Kauana dos Santos kdossantos@vmware.com

WHAT is this change about?

We want to avoid having .s in pod names because some platforms don't allow that.

#175169873

Does this PR introduce a change to config/values.yml?

No

Acceptance Steps

When I have a cluster with cf-for-k8s deployed And I describe the restart-sidecar-job with k get po -n cf-workloads | grep "restart" Then I see that the pod name is restart-workloads-for-istio1-7-1-<hash> and not restart-workloads-for-istio1.7.1-<hash>

Tag your pair, your PM, and/or team

@kauana @cloudfoundry/cf-for-k8s-networking

cf-gitbot commented 4 years ago

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/175174417

The labels on this github issue will be updated when the story is started.

cf-rel-int-status-bot commented 4 years ago

Hello friend, it looks like your pull request has failed one or more of our checks. Please take a look! :eyes:

cf-rel-int-status-bot commented 4 years ago

Hello friend, it looks like your pull request has failed one or more of our checks. Please take a look! :eyes:

cf-rel-int-status-bot commented 4 years ago

Hello friend, it looks like your pull request has failed one or more of our checks. Please take a look! :eyes:

ericpromislow commented 4 years ago

The check-vendir-sync job failed because we run build/istio/build.sh after running vendir sync.

In particular, https://github.com/cloudfoundry/cf-for-k8s/blob/db6f0aa9a6db96ba43b0249fdf11adeb83a8fc32/build/istio/build.sh#L9-L10 rewrites the version field

This replaces whatever follows the return stmt in config/istio/upgrade-istio-sidecars-job.yml with the dotted version.

I was asking people on the relint team, but it looks like this code came from cf-k8s-networking back on Sept 9, 2020. Is it still needed? (And if so, why?)

KauzClay commented 4 years ago

@ericpromislow Yeah, that sed command was so we could change istio version in one place and have our tooling replace it in all the necessary places.

We changed the regex in the sed to be more specific, and tweaked the overlay. I think it should work now.

cf-rel-int-status-bot commented 4 years ago

Hello friend, it looks like your pull request has failed one or more of our checks. Please take a look! :eyes:

davewalter commented 4 years ago

For historical record, the rename of the restart-workloads-for-istio1-7-3 job caused app downtime during the upgrade. This was expected as all application pods were restarted as part of the upgrade.