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.

Sign up for GitHub

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

Jump to bottom

Always set a field manager name #2271

Merged
merged 4 commits into from
Dec 19, 2022
Merged

Always set a field manager name #2271

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9cda592
Always set a field manager name
lblackstone Dec 15, 2022
69a6f94
Changelog
lblackstone Dec 15, 2022
fe7796b
Improve changelog description
lblackstone Dec 16, 2022
df48701
Change fieldManagerName to be a provider method
lblackstone Dec 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

- Add `PULUMI_K8S_ENABLE_PATCH_FORCE` env var support (https://github.com/pulumi/pulumi-kubernetes/pulls/2260)
- Add link to resolution guide for SSA conflicts (https://github.com/pulumi/pulumi-kubernetes/pulls/2265)
- Always set a field manager name (https://github.com/pulumi/pulumi-kubernetes/pulls/2271)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say it's not that obvious why this helps, so perhaps it would be better to phrase this in terms of what it fixes for people, in the changelog.


## 3.23.0 (December 8, 2022)

Expand Down
8 changes: 6 additions & 2 deletions provider/pkg/await/await.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,9 @@ func Creation(c CreateConfig) (*unstructured.Unstructured, error) {
ssaConflictDocLink, err)
}
} else {
var options metav1.CreateOptions
options := metav1.CreateOptions{
FieldManager: c.FieldManager,
}
if c.Preview {
options.DryRun = []string{metav1.DryRunAll}
}
Expand Down Expand Up @@ -452,7 +454,9 @@ func Update(c UpdateConfig) (*unstructured.Unstructured, error) {
return nil, err
}

var options metav1.PatchOptions
options := metav1.PatchOptions{
FieldManager: c.FieldManager,
}
if c.Preview {
options.DryRun = []string{metav1.DryRunAll}
}
Expand Down
19 changes: 12 additions & 7 deletions provider/pkg/provider/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1552,7 +1552,7 @@ func (k *kubeProvider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*p
newInputs.GetNamespace(), newInputs.GetName())
}

fieldManager := fieldManagerName(nil, newResInputs, newInputs)
fieldManager := fieldManagerName(nil, newResInputs, newInputs, k.serverSideApplyMode)

// Try to compute a server-side patch.
ssPatch, ssPatchBase, ssPatchOk, err := k.tryServerSidePatch(oldInputs, newInputs, gvk, fieldManager)
Expand Down Expand Up @@ -1749,7 +1749,7 @@ func (k *kubeProvider) Create(
}

initialAPIVersion := newInputs.GetAPIVersion()
fieldManager := fieldManagerName(nil, newResInputs, newInputs)
fieldManager := fieldManagerName(nil, newResInputs, newInputs, k.serverSideApplyMode)

if k.yamlRenderMode {
if newResInputs.ContainsSecrets() {
Expand Down Expand Up @@ -1974,7 +1974,7 @@ func (k *kubeProvider) Read(ctx context.Context, req *pulumirpc.ReadRequest) (*p
if err != nil {
return nil, err
}
fieldManager := fieldManagerName(nil, oldState, oldInputs)
fieldManager := fieldManagerName(nil, oldState, oldInputs, k.serverSideApplyMode)

if k.yamlRenderMode {
// Return a new "checkpoint object".
Expand Down Expand Up @@ -2243,8 +2243,8 @@ func (k *kubeProvider) Update(
return nil, err
}

fieldManagerOld := fieldManagerName(nil, oldState, oldInputs)
fieldManager := fieldManagerName(nil, oldState, newInputs)
fieldManagerOld := fieldManagerName(nil, oldState, oldInputs, k.serverSideApplyMode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does k.serverSideApply also have an old value and a new value, in the sense of it being switched on since the last run? Would that matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

The value can change, but the provider logic should be able to cope with it. Generally speaking, the provider will leave values alone that are already set, but would use a different code path for computing diffs.

fieldManager := fieldManagerName(nil, oldState, newInputs, k.serverSideApplyMode)

if k.yamlRenderMode {
if newResInputs.ContainsSecrets() {
Expand Down Expand Up @@ -2421,7 +2421,7 @@ func (k *kubeProvider) Delete(ctx context.Context, req *pulumirpc.DeleteRequest)
if err != nil {
return nil, err
}
fieldManager := fieldManagerName(nil, oldState, oldInputs)
fieldManager := fieldManagerName(nil, oldState, oldInputs, k.serverSideApplyMode)
resources, err := k.getResources()
if err != nil {
return nil, pkgerrors.Wrapf(err, "Failed to fetch OpenAPI schema from the API server")
Expand Down Expand Up @@ -2903,7 +2903,12 @@ func initialAPIVersion(state resource.PropertyMap, oldConfig *unstructured.Unstr
// 1. Resource annotation (this will likely change to a typed option field in the next major release)
// 2. Value from the Pulumi state
// 3. Randomly generated name
func fieldManagerName(randomSeed []byte, state resource.PropertyMap, inputs *unstructured.Unstructured) string {
func fieldManagerName(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it the case that for each invocation of fieldManagerName, you already know something about where the result should come from? For instance, in Create there's no existing state, so the field manager name must come from the inputs (or be a const).

If so, then ignoring that information and evaluating it again means it relies on coincidence -- it assumes that this procedure will make the same decisions as the calling code. If the field manager name must be in the state, for example, then it shouldn't fall back on cons'ing a new random name, it should return an error.

I have not internalised how this provider code all works, or even read all of it, so I may be way off beam!

Copy link
Contributor

Choose a reason for hiding this comment

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

A less dramatic observation: serverSideApplyMode is always just k.serverSideApplyMode; perhaps this should be a method of the provider, so this goes through a kind of internal dispatch rather than relying on the caller to provide the (right) value.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if I completely follow your question. The intent of this method is to ensure that a fieldManager name exists according to the precedence indicated in the docstring. If the value isn't specified, then one will be automatically assigned during resource creation.

Good call about changing this to be a provider method. I'll update that.

randomSeed []byte, state resource.PropertyMap, inputs *unstructured.Unstructured, serverSideApplyMode bool,
) string {
if !serverSideApplyMode {
return "pulumi-kubernetes"
}
if v := metadata.GetAnnotationValue(inputs, metadata.AnnotationPatchFieldManager); len(v) > 0 {
return v
}
Expand Down