fix exec node resource calculation on non-isolated CRI-powered job environment #277
Conversation
| @@ -146,7 +146,7 @@ func (n *baseExecNode) doBuildCRISidecar(envSpec *ytv1.JobEnvironmentSpec, podSp | |||
| } | |||
|
|
|||
| if n.spec.JobResources != nil { | |||
| jobsContainer.Resources = *n.spec.JobResources | |||
| jobsContainer.Resources = *n.spec.JobResources.DeepCopy() | |||
There was a problem hiding this comment.
I haven't found where the value was overriden in code though.
But we shooting ourselves in a leg using mutable instanceSpec object.
There was a problem hiding this comment.
right, this specific field isn't changed anywhere in the code. I just 'fixed' it to be the same looking as the node resources where the bug has appeared.
| } | ||
| } | ||
|
|
||
| func getYtsaurus() *ytv1.Ytsaurus { |
There was a problem hiding this comment.
Please DO NOT COPY-PASTE such test assets. It will be impossible to maintain.
There was a problem hiding this comment.
@Kruft can you please fix this tests with reusing CRD building from another package?
| } | ||
| } | ||
|
|
||
| func getYtsaurus() *ytv1.Ytsaurus { |
There was a problem hiding this comment.
@Kruft can you please fix this tests with reusing CRD building from another package?
| cfgen: g, | ||
| spec: &execNodeSpec, | ||
| sidecarConfig: nil, | ||
| } |
There was a problem hiding this comment.
Also this test looks rather fragile since it tests low-level implementation of the component and will often broke on refactorings. Does it really test something that couldn't be tested in ytconfig tests?
There was a problem hiding this comment.
I've reverted this particular test file #281
Could you please send another PR with test if you feel it is needed (I expect them to be among ytconfig tests and without manipulating base struct)?
…vironment (ytsaurus#277) * stage * stage * stage
No description provided.