Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Clear managed fields on FlyteWorkflow CRD #344

Merged
merged 1 commit into from
Oct 20, 2021

Conversation

hamersaw
Copy link
Contributor

TL;DR

During each FlyteWorkflow CRD update we check for existing managed field entities and set the field to a list with a single empty instance. K8s server side apply uses this to clear managed fields. Managed fields are unused since Flyte ensures only a single FlytePropeller instance manages each FlyteWorkflow. Removing them reduces CRD size to improve etcd read / write performance.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

^^^

Tracking Issue

fixes flyteorg/flyte#1714

Follow-up issue

NA

…rd managed fields

Signed-off-by: Daniel Rammer <daniel@union.ai>
@codecov
Copy link

codecov bot commented Oct 19, 2021

Codecov Report

Merging #344 (1deceef) into master (ae70176) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

// FlyteWorkflow CRDs are only managed by a single FlytePropeller instance and therefore the managed fields paradigm
// does not add useful functionality. Clearing them reduces CRD size, improving etcd I/O performance.
if len(workflow.ObjectMeta.ManagedFields) > 0 {
workflow.ObjectMeta.ManagedFields = workflow.ObjectMeta.ManagedFields[:1]
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 not just set it to an empty array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Note that setting the managedFields to an empty list will not reset the field. This is on purpose, so managedFields never get stripped by clients not aware of the field." (https://kubernetes.io/docs/reference/using-api/server-side-apply/#clearing-managedfields)

We need to set it to a 1 element array with an empty ManagedField entry.

Copy link
Contributor

Choose a reason for hiding this comment

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

wow, good to know

@kumare3 kumare3 merged commit 189452f into flyteorg:master Oct 20, 2021
eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
…rd managed fields (flyteorg#344)

Signed-off-by: Daniel Rammer <daniel@union.ai>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core][Performance] Clear managed fields from FlyteWorkflow CRD
2 participants