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

Fix a bug in zoneToRegion which didn't work if multiple zones where passed separated by __ #2731

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

viveksinghggits
Copy link
Contributor

Change Overview

There was a bug in zoneToRegion function because of which the PV resources were not being created with correct nodeAffinity set. zoneToRegion tried to get regions from passed zone just by removing -[onechar] from the zone. For example if we passed the zone us-west1-b this function would return us-west1 by removing -b. This would work properly in the cases where just one zone is given to the function.

But if more than one zones are given to the function (separated by __), this function didn't work properly. Because for the input us-west1-a__us-west2-a it returned us-west1-a__us-west2 (using existing logic), which is incorrect. As we can see the -a was not reomved from first zone. This commit doesn't introduce the separator __, it was already part of code and zones were being passed to this funciton separated by __.

This commit fixes above problem by making sure that we split the zones and then remove -char from the zone and add the regions together again using __ sep.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

  • fixes #issue-number

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E
go test -check.f "TestVolSuite.TestZoneToRegion" -count 10
OK: 1 passed
OK: 1 passed
OK: 1 passed
OK: 1 passed
OK: 1 passed
OK: 1 passed
OK: 1 passed
OK: 1 passed
OK: 1 passed
OK: 1 passed
PASS
ok  	github.com/kanisterio/kanister/pkg/kube/volume	0.520s

There was a bug in `zoneToRegion` function because of which the PV resources
were not being created with correct `nodeAffinity` set. `zoneToRegion` tried
to get regions from  passed zone just by removing `-[onechar]` from the zone.
For example if we passed the zone `us-west1-b` this function would return
`us-west1` by removing `-b`. This would work properly in the cases where just
one zone is given to the function.

But if more than one zones are given to the function (separated by `__`), this
function didn't work properly. Because for the input `us-west1-a__us-west2-a`
it returned `us-west1-a__us-west2` (using existing logic), which is incorrect.
As we can see the `-a` was not reomved from first zone. This commit doesn't
introduce the separator `__`, it was already part of code and zones were being
passed to this funciton separated by `__`.

This commit fixes above problem by making sure that we split the zones and then
remove `-char` from the zone and add the regions together again using `__` sep.
@infraq infraq added this to In Progress in Kanister Mar 8, 2024
@viveksinghggits viveksinghggits changed the title Fix a but in zoneToRegion which didn't work if multiple zones where passed separated by __ Fix a bug in zoneToRegion which didn't work if multiple zones where passed separated by __ Mar 8, 2024
Kanister automation moved this from In Progress to Reviewer approved Mar 8, 2024
@mergify mergify bot merged commit 79fb8cc into master Mar 8, 2024
14 checks passed
Kanister automation moved this from Reviewer approved to Done Mar 8, 2024
@mergify mergify bot deleted the fix-zonetoregion branch March 8, 2024 17:34
Copy link
Contributor

@e-sumin e-sumin left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants