Skip to content

PWX-30230 : Create all px configmaps in same namespace as portworx#314

Open
nikita-bhatia wants to merge 9 commits intomasterfrom
cm_namespace
Open

PWX-30230 : Create all px configmaps in same namespace as portworx#314
nikita-bhatia wants to merge 9 commits intomasterfrom
cm_namespace

Conversation

@nikita-bhatia
Copy link
Contributor

@nikita-bhatia nikita-bhatia commented Apr 28, 2023

https://portworx.atlassian.net/browse/PWX-30230

This PR is for migration of namespace from 'kube-system' to 'portworx'. To support this,

  • Added coreConfigMap struct as wrapper function for existing configmap.
  • Added Instance() to decide which configmap to use, and handle upgrade scenarios

Logic to update copylock config map entry once upgrade is finished will be handled in porx code.

Signed-off-by: Nikita Bhatia <nbhatia@purestorage.com>
nikita-bhatia and others added 5 commits April 28, 2023 16:33
Signed-off-by: Nikita Bhatia <nbhatia@purestorage.com>
Signed-off-by: Nikita Bhatia <nbhatia@purestorage.com>
Signed-off-by: Divyam Goel <dgoel@purestorage.com>
Signed-off-by: Nikita Bhatia <nbhatia@purestorage.com>
Signed-off-by: Divyam Goel <dgoel@purestorage.com>
Signed-off-by: Nikita Bhatia <nbhatia@purestorage.com>
Signed-off-by: Divyam Goel <dgoel@purestorage.com>
Signed-off-by: Nikita Bhatia <nbhatia@purestorage.com>
Signed-off-by: Nikita Bhatia <nbhatia@purestorage.com>
Signed-off-by: Nikita Bhatia <nbhatia@purestorage.com>
},
Data: data,
if c.pxNs == c.config.nameSpace {
//fresh install ot upgrade completed
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: .. or.. ?

name, err)
}
existingConfig := c.config
c.copylock.Lock(uuid.New().String())
Copy link
Contributor

Choose a reason for hiding this comment

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

The parameter to Lock is the owner. It should be the node ID. If you generate a new UUID everytime, we wouldn't know from the logs who is holding it.

log.Errorf("Error during fetching data from copy lock %s", err)
return existingConfig
}
status := lockMap[upgradeCompletedStatus]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we explicitly have a check for non existence ?
status, ok :=

maxConflictRetries = 3
upgradeCompletedStatus = "UPGRADE_DONE"
trueString = "true"
falseString = "false"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: falseString doesn't seem to be used anywhere. Remove?

}
cm, err := New("px-configmaps-test", configData, lockTimeout, 5, 0, 0)
cm, err := New("px-configmaps-test", configData, lockTimeout, 5, 0, 0, "test-ns")
fmt.Println("cm : ", cm)
Copy link
Contributor

Choose a reason for hiding this comment

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

was this for dev debugging? Remove?

return existingConfig
}
status := lockMap[upgradeCompletedStatus]
if status == trueString {
Copy link
Contributor

Choose a reason for hiding this comment

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

How are we handling IN_PROGRESS state.
Let's say N1 is doing the copy. At the same time N2 is trying to update some value usingUpdate() function. According to this implementation, on N2 existingConfig will be returned and could be updating while N1 could be deleting the whole configmap at line 52. Don't we need to wait if a copy is IN_PROGRESS?

@nrevanna
Copy link
Contributor

Can you add testing information. Also share a testplan. This one will need to tested well. Would prefer to have it all documented in testrail.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants