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.

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

Write to intermediate file before moving to state file #755

Merged
merged 6 commits into from
Jan 8, 2021

Conversation

tamilmani1989
Copy link
Member

@tamilmani1989 tamilmani1989 commented Dec 16, 2020

Reason for Change:

Write to intermediate file before moving to state file

Issue Fixed:

IP leak fixes
Ipam pool fixes

Requirements:

Notes:

@codecov
Copy link

codecov bot commented Dec 16, 2020

Codecov Report

Merging #755 (789fd74) into master (2e2efa7) will increase coverage by 1.75%.
The diff coverage is 51.11%.

@@            Coverage Diff             @@
##           master     #755      +/-   ##
==========================================
+ Coverage   39.61%   41.36%   +1.75%     
==========================================
  Files          82      142      +60     
  Lines       10800    13472    +2672     
==========================================
+ Hits         4278     5573    +1295     
- Misses       6017     7199    +1182     
- Partials      505      700     +195     

Copy link
Contributor

@ramiro-gamarra ramiro-gamarra left a comment

Choose a reason for hiding this comment

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

Some comments on the file flush, I'm not familiar with the other changes

store/json.go Outdated Show resolved Hide resolved
store/json.go Outdated Show resolved Hide resolved
store/json.go Outdated Show resolved Hide resolved
platform/os.go Outdated
return nil
}

isExist, _ := CheckIfFileExists(dirPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

check error?

Copy link
Member Author

Choose a reason for hiding this comment

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

same here...i can print err but i cannot validate error

Copy link
Member Author

Choose a reason for hiding this comment

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

checked this link: https://stackoverflow.com/questions/37932551/mkdir-if-not-exists-using-golang . let me validate error also and createdirectory

Copy link
Member

@matmerr matmerr left a comment

Choose a reason for hiding this comment

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

followed up offline, approving invoker changes

@tamilmani1989 tamilmani1989 merged commit 98f838e into Azure:master Jan 8, 2021
@AbelHu
Copy link
Member

AbelHu commented Jan 19, 2021

@tamilmani1989 @ramiro-gamarra @matmerr When will the new Azure CNI version be released? One AKS Windows user hit the issue 3 times in one week that the Azure CNI file is invalid.

@zhiweiv
Copy link
Contributor

zhiweiv commented Jan 20, 2021

@AbelHu
Today 1.2.2 released with this fix, can you merge this to AKS asap, the invalid azure cni state file issue still occur frequently in our clusters, ip leak issue also occur several times.

We'd like to verify if this fix works.

@zhiweiv zhiweiv mentioned this pull request Jan 20, 2021
@AbelHu
Copy link
Member

AbelHu commented Jan 20, 2021

@zhiweiv We are working on this. We will try our best to update it in AKS ASAP.

@AbelHu
Copy link
Member

AbelHu commented Jan 26, 2021

@zhiweiv azure cni v1.2.2 should be available in Azure global regions now. You can upgrade it by upgrading cluster or updating node images of Windows agent pools.

@zhiweiv
Copy link
Contributor

zhiweiv commented Jan 27, 2021

Upgraded the problem AKS to lastest node image, it is azure cni 1.2.2 now. I will keep monitoring this issue.

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

Successfully merging this pull request may close these issues.

5 participants