Fix Topology Manager policy and scope not being updated after NRT creation

[PiotrProkop]

fixes #1254

[netlify]

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	6d98b61
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/64b945789a83210008a33c92
😎 Deploy Preview	https://deploy-preview-1256--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[PiotrProkop]

/cc @marquiz @ffromani

[ArangoGutierrez]

/test pull-node-feature-discovery-build-image-cross-generic

[ffromani]

Ideally, I'd prefer having a goroutine waiting for changes in the kubeletconfig and re-reading it if needed and only if needed, avoding constant and costly polling. IIRC we use already fsnotify in NFD so this would mean no new deps. Have you explored this option? how hard would it be to implement?

[PiotrProkop]

I was thinking about it but NFD supports reading kubelet either from file or from http endpoint and I don't know if /configz endpoint is watchable. Should I implement this in seperate goroutine only when reading kubelet config and make it poll based when using http endpoint to obtain kubelet config?

[PiotrProkop]

I feel like if we want to support both endpoints and not to overcomplicate the code. We can spin up another goroutine that will check kubelet config on startup and every 5mins(or make it configurable) to reduce costly poling and patch only TopologyPolices or attributes field?

[ffromani]

Fair point. I'm hesitant to add kubelet config read in this path, this is why I mentioned a different goroutine.
Let me review the code again and think a bit more.

[PiotrProkop]

I feel like spinning up another goroutine which will be updating NRT object can lead to conflicts when updating same resource it would be best to have one func that's responsible for managing NRT. We can also consider such implementation:

-       policy, scope, err := w.detectTopologyPolicyAndScope()
-       if err != nil {
-               return fmt.Errorf("failed to detect TopologyManager's policy and scope: %w", err)
-       }
-
-       tmAttributes := createTopologyAttributes(policy, scope)
-       deprecatedTopologyPolicies := []string{string(topologypolicy.DetectTopologyPolicy(policy, scope))}
-
        nrt, err := cli.TopologyV1alpha2().NodeResourceTopologies().Get(context.TODO(), w.nodeName, metav1.GetOptions{})
        if errors.IsNotFound(err) {
+               policy, scope, err := w.detectTopologyPolicyAndScope()
+               if err != nil {
+                       return fmt.Errorf("failed to detect TopologyManager's policy and scope: %w", err)
+               }
+
+               tmAttributes := createTopologyAttributes(policy, scope)
+               deprecatedTopologyPolicies := []string{string(topologypolicy.DetectTopologyPolicy(policy, scope))}
+
                nrtNew := v1alpha2.NodeResourceTopology{
                        ObjectMeta: metav1.ObjectMeta{
                                Name: w.nodeName,
@@ -237,15 +239,28 @@ func (w *nfdTopologyUpdater) updateNodeResourceTopology(zoneInfo v1alpha2.ZoneLi
        nrtMutated := nrt.DeepCopy()
        nrtMutated.Zones = zoneInfo

-       attributes := append(scanResponse.Attributes, tmAttributes...)
-       updateAttributes(&nrtMutated.Attributes, attributes)
+       if w.lastUpdateTimestamp.IsZero() || time.Since(w.lastUpdateTimestamp).Minutes() > float64(5) {
+               policy, scope, err := w.detectTopologyPolicyAndScope()
+               if err != nil {
+                       return fmt.Errorf("failed to detect TopologyManager's policy and scope: %w", err)
+               }

-       nrtMutated.TopologyPolicies = deprecatedTopologyPolicies
+               tmAttributes := createTopologyAttributes(policy, scope)
+               deprecatedTopologyPolicies := []string{string(topologypolicy.DetectTopologyPolicy(policy, scope))}
+
+               attributes := append(scanResponse.Attributes, tmAttributes...)
+               updateAttributes(&nrtMutated.Attributes, attributes)
+
+               nrtMutated.TopologyPolicies = deprecatedTopologyPolicies
+       }

        nrtUpdated, err := cli.TopologyV1alpha2().NodeResourceTopologies().Update(context.TODO(), nrtMutated, metav1.UpdateOptions{})
        if err != nil {
                return fmt.Errorf("failed to update NodeResourceTopology: %w", err)
        }
+
+       w.lastUpdateTimestamp = time.Now()
+

or move reading scope and policy into seperate goroutine and keep them in shared state somewhere?

[PiotrProkop]

@ffromani to reduce number of times we need to read Kubelet config file and minimize complexity. I have modified the code only to reread Kubelet config when Interval based event is triggered, which default to 1 min(somewhat low). Should we introduce another ticker?
I feel like we should implement a better solution to this error but I am unsure if we need to do it in this PR.

[ffromani]

thanks @PiotrProkop my main issue was finding time to review, I'll have another pass ASAP. I tend to agree without a some redesign (which is better served on a separate PR) we can't do much better than the current approach.

[ArangoGutierrez]

/milestone v0.14

[marquiz]

Thanks @PiotrProkop for the enhancement. I have just a few notes after a quick read-through, relying mostly on @ffromani here :)

One ask: you have nice description in the related github issue, could you take some of that to the commit message?

[marquiz]

Do we get this on the very first run when starting up?

[PiotrProkop]

Checking now.

[PiotrProkop]

If I understand correctly current behavior there is no first run during startup. The first update happens either after sleepInterval or when nfd detects changes to checkpoint files.

[PiotrProkop]

Thanks @PiotrProkop for the enhancement. I have just a few notes after a quick read-through, relying mostly on @ffromani here :)

One ask: you have nice description in the related github issue, could you take some of that to the commit message?

Thanks for the review! Commit message changed.

[marquiz]

Thx for the updates @PiotrProkop. I don't think I have much to add. I'll leave this to @ffromani now and will approve when he's happy

[marquiz]

ping @ffromani

[ffromani]

ping @ffromani

Hi! sorry for the delay I'll review by the end of this week (July 28)

[marquiz]

Hi! sorry for the delay I'll review by the end of this week (July 28)

Thanks @ffromani, so take your time. No panic with this (yet), just wanted to remind so that this wouldn't be the last one pending

[ffromani]

/lgtm

Designwise I think this is good enough to fix the issue at hand. I don't see any blocking issue. I'm under the impression that we hit the tipping point (or we're very close to it) and that the environment changed enough to warrant a rethink of the flow and a redesign in a more cohesive way,

But it would be unfair to account his work on this PR, so let's go with this approach. I can't see obvious issue, so LGTM.

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: d92efb0eb0bdcd13ab84ddf4f2b39e39af9f0cbe

[marquiz]

Thanks @ffromani. I agree that going forward some re-architecting would be beneficial, e.g. detecting kubelet restarts or smth

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz, PiotrProkop

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:

• OWNERS [marquiz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[marquiz]

/retest

[PiotrProkop]

 /home/prow/go/src/github.com/kubernetes-sigs/node-feature-discovery/pkg/utils/memory_resources.go:54:17: n.Name undefined (type fs.DirEntry has no field or method Name) 

should I rebase my branch to fix it? @marquiz

[marquiz]

should I rebase my branch to fix it? @marquiz

Hmm, no, the logcheck seems to be flaky 🧐
/retest

[codecov]

Codecov Report

Merging #1256 (6d98b61) into master (fd0ba3f) will increase coverage by 2.77%.
Report is 20 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1256      +/-   ##
==========================================
+ Coverage   29.19%   31.97%   +2.77%     
==========================================
  Files          52       54       +2     
  Lines        7234     7638     +404     
==========================================
+ Hits         2112     2442     +330     
- Misses       4896     4959      +63     
- Partials      226      237      +11     

Files Changed	Coverage Δ
cmd/nfd-topology-updater/main.go	45.83% <0.00%> (+12.83%)	⬆️
pkg/nfd-topology-updater/nfd-topology-updater.go	3.25% <0.00%> (-1.07%)	⬇️

... and 5 files with indirect coverage changes