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

fix: clean up stale CNI data regardless of HNS state #3822

Merged
merged 3 commits into from
Sep 14, 2020

Conversation

ashvindeodhar
Copy link
Member

@ashvindeodhar ashvindeodhar commented Sep 14, 2020

fix: cleanup stale CNI data regardless of HNS state
Currently CNI stale data is cleaned up only when a stale HNS network is found.
This change removes that unnecessary dependency and cleans up the stale CNI data
regardless of the HNS state. This is needed when HNS network is not persisted across the reboots.

Reason for Change:

Issue Fixed:

Fixes Azure/azure-container-networking#642

Requirements:

Notes:

@welcome
Copy link

welcome bot commented Sep 14, 2020

💖 Thanks for opening your first pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix. Examples of commit messages with semantic prefixes: - fix: change azure disk cachingMode to ReadOnly - feat: make maximumLoadBalancerRuleCount configurable - docs: add note on AKS Engine and AKS relationship
Make sure to check out the developer guide for guidance on testing your change.

@acs-bot acs-bot added the size/M label Sep 14, 2020
@ashvindeodhar ashvindeodhar changed the title Clean stale CNI data fix: clean up stale CNI data regardless of HNS state Sep 14, 2020
Currently CNI stale data is cleaned up only when a stale HNS network is found.
This change removes that unnecessary dependency and cleans up the stale CNI data
regardless of the HNS state. This is needed when HNS network is not persisted across the reboots.
@ashvindeodhar
Copy link
Member Author

/assign @marosset

@jackfrancis
Copy link
Member

Thanks @ashvindeodhar, is this meant to be a Windows-only change? There is no equivalent Linux change to benefit those scenarios?

@ashvindeodhar
Copy link
Member Author

@jackfrancis yes this is for windows-only. There is no change for linux scenario since linux removes all the CNI stale data upon reboot.

@marosset
Copy link
Contributor

/azp run pr-e2e

@marosset
Copy link
Contributor

/lgtm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@acs-bot acs-bot added the lgtm label Sep 14, 2020
@marosset
Copy link
Contributor

@AbelHu @mainred as FYI

@acs-bot
Copy link

acs-bot commented Sep 14, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashvindeodhar, marosset

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov
Copy link

codecov bot commented Sep 14, 2020

Codecov Report

Merging #3822 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3822   +/-   ##
=======================================
  Coverage   73.19%   73.19%           
=======================================
  Files         148      148           
  Lines       25394    25394           
=======================================
  Hits        18587    18587           
  Misses       5671     5671           
  Partials     1136     1136           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 735f843...ae016c2. Read the comment docs.

@ashvindeodhar
Copy link
Member Author

@marosset / @jackfrancis Do you know which AKS release will this get into? I am looking to get some idea as to when (ETA) will this is available to the customers.

@jackfrancis
Copy link
Member

@xuto2 are you able to answer @ashvindeodhar's question?

@xuto2
Copy link
Contributor

xuto2 commented Sep 14, 2020

@AbelHu Can you comment on the above question? It's an issue for windows only.

@jackfrancis jackfrancis merged commit 5d449a3 into Azure:master Sep 14, 2020
@welcome
Copy link

welcome bot commented Sep 14, 2020

Congrats on merging your first pull request! 🎉🎉🎉

@AbelHu
Copy link
Member

AbelHu commented Sep 15, 2020

@ashvindeodhar I think that aks-e will sign this script and publish a new Windows signed binary. After that, AKS can pick up it. Please let me know when the new signed binary is ready. @marosset. Thanks.

@mainred
Copy link
Member

mainred commented Sep 15, 2020

Want to confirm from @ashvindeodhar, we can resolve Azure/azure-container-networking#642 by only this PR, right?
If so, Windows signed binary for CNI is required, but we need to include changes in this PR to AKS used branch.
Sorry my bad, please forget this message, though it was part of Windows CSE filed.

@zhiweiv
Copy link
Contributor

zhiweiv commented Sep 16, 2020

Just FYI, the issue in AKS is Azure/AKS#1772, it is great to update the ETA there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to start network plugin, err:invalid character '\x00' looking for beginning of value.
9 participants