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
Bug 1901107: fix pod donut information #6864
Bug 1901107: fix pod donut information #6864
Conversation
/kind bug |
4698ebc
to
68fb3cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@debsmita1 When I save the edit form the update strategy resets to RollingUpdate
Try these steps:
- Create a deployment
- Now, Update the strategy to Recreate.
- Edit the form increase the replicas
- Save the Form
Notice that the update strategy is nowRollingUpdate
68fb3cc
to
e3aa43b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I save the edit form the update strategy resets to RollingUpdate
Try these steps:
Create a deployment
Now, Update the strategy to Recreate.
Edit the form increase the replicas
Save the Form
Notice that the update strategy is now RollingUpdate
@Debsmita Please check the same for DeploymentConfig as well.
...(originalDeployment?.spec.strategy.type && { | ||
strategy: { type: originalDeployment?.spec.strategy.type }, | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there might be other information in the strategy object. so we want to spread the whole strategy
object here.
example:
strategy:
type: RollingUpdate
rollingUpdate:
maxUnavailable: 25%
maxSurge: 25%
...(originalDeployment?.spec.strategy.type && { | ||
strategy: { type: originalDeployment?.spec.strategy.type }, | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
e3aa43b
to
6586047
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@debsmita1 I am seeing animation for the RollingUpdate
but not for the Recreate
strategy
Steps are same in both cases Edit the resource then change replicas and resource Limits 0,1,0,1
it's because the previous replicaset is returned as empty, what we are seeing in the Recreate Updatestrategy is the current replicaset |
...(originalDeployment?.spec.strategy && { | ||
strategy: originalDeployment?.spec.strategy, | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably ok for now because we seem to be losing the previous setting. However we should ensure that we do not override any other properties by accident.
I believe this is an area that @divyanshiGupta will be addressing in another issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@debsmita1 If you notice we are not making any changes based on the originalResource
while creating the object for the newResource
as these utilities are used for both create and edit flows and to keep the newResource
object clean and also as Christian mentioned above to avoid accidental overrides to any other properties. You should make the changes in the mergeData function which is used to merge the old and new resources and also in cases where we want to keep some properties of old resource. If you see in mergeData function we already have a check
if (mergedData?.spec?.strategy) {
mergedData.spec.strategy = newResource.spec.strategy;
}
for your usecase you just need to change this check to something like
if (mergedData?.spec?.hasOwnProperty('strategy')) {
mergedData.spec.strategy = newResource.spec?.strategy ?? originalResource?.spec?.strategy;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the point of these if
conditions in mergeData
to intentionally override the merged values?
It seems a bit odd that the condition checks the merged state instead of the newResource
state before entering the condition. Why is that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usecase I can think of where we need to check the mergedData
values is when the originalResource
has a property but during edit that property was removed but when we merged the two objects the property again got added and therefore checking the mergedData
if it has that property we can update it with newResource
property value which in this case will be undefined. Although for this usecase where strategy for the new resource (d/dc) is always going to be undefined, the merged data will have the original resource strategy which is required here and we can change this check to
if (newResource.spec?.hasOwnProperty('strategy')) {
mergedData.spec.strategy = newResource.spec.strategy;
}
to update the strategy with newResource strategy value in case of buildStrategy which can be updated using the edit flows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@divyanshiGupta when editing the deployment (which has Recreate strategy), the strategy property is not added in the new Resource obj that we create, and the existing mergeData util reads from the newResource.spec.strategy
which doesn't exist as a result the merged resource has spec.strategy as undefined. So according to me we must consider reading from the original resource. WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@debsmita1 as I mentioned above since strategy is going to be undefined in the new resource so during merge strategy prop of original resource will be preserved. You can also try that out. We need to add check on new resource since if we have strategy in it we want the mergedData to have the new strategy in case of buildStrategy as it can be updated using edit flows unlike d/dc strategy in which case the new resource strategy is always going to be undefined and hence the original strategy will be used.
Not sure if it should be part of this fix or not.
I expect the right circle to say scaling to 3. And shouldn't the left say scaling to 0 if it is rolling? When I performed the same test with a D, it did the following. Maybe we should create matrix of expectations for the left and right donuts during an update per strategy. cc @spadgett |
6586047
to
4a3bf04
Compare
@christianvogt I am referring to this PR #2496 to understand the pod visuals, and it looks like in DC's case I would need to fix the pod text |
@christianvogt Below are the visuals that I captured with my changes, the visuals depend on when I am setting the resource limits. In the 1st screenshot, it looks like the pod donut on the left could never scale up to the desired number of replicas For Deployment, edit app before build is complete For Deployment, edit App after build is complete |
Not sure now what I did to create that scenario anymore. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt, debsmita1 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 |
/cherry-pick release-4.6 |
@andrewballantyne: new pull request created: #7313 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. |
/retitle Bug 1901107: fix pod donut information |
@debsmita1: All pull requests linked via external trackers have merged: Bugzilla bug 1901107 has been moved to the MODIFIED state. 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. |
/bugzilla refresh |
@andrewballantyne: Bugzilla bug 1901107 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state. 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. |
fixes:
https://issues.redhat.com/browse/ODC-3292
root analysis:
Solution Description:
Screeshot:
Edit the update strategy to Recreate
Edit the update strategy to Recreate