AgentBaker icon indicating copy to clipboard operation
AgentBaker copied to clipboard

feat: Don't set feature gate CustomCPUCFSQuotaPeriod, Get them from kubelet configs

Open gaopenghigh opened this issue 1 year ago • 7 comments

What type of PR is this?

/kind feature

What this PR does / why we need it:

Currently, if CPUCFSQuotaPeriod are set in CustomKubeletConfigs, agent baker set feature gates "CustomCPUCFSQuotaPeriod =true", this PR removes this logic since the feature gate is already set by RP.

Which issue(s) this PR fixes:

Fixes #

Requirements:

Special notes for your reviewer:

Release note:

none

gaopenghigh avatar Mar 30 '24 07:03 gaopenghigh

Pull Request Test Coverage Report for Build 9966808887

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 71.005%

Totals Coverage Status
Change from base Build 9966582612: -0.02%
Covered Lines: 2564
Relevant Lines: 3611

💛 - Coveralls

coveralls avatar Mar 30 '24 07:03 coveralls

we should probably wait on merging on this until the RP change has rolled out to all regions, otherwise we'll risk those feature gates not getting properly set

cameronmeissner avatar Apr 02 '24 01:04 cameronmeissner

also we don't generally link to ADO PRs and things of that nature here since this is OSS :)

cameronmeissner avatar Apr 02 '24 01:04 cameronmeissner

Yes this one should wait for the RP change.

gaopenghigh avatar Apr 02 '24 14:04 gaopenghigh

The RP change has already rolled out to all regions, now we can merge this, @cameronmeissner could you help to review again?

gaopenghigh avatar May 10 '24 04:05 gaopenghigh

The RP change has already rolled out to all regions, now we can merge this, @cameronmeissner could you help to review again?

do we still want/need to merge this?

cameronmeissner avatar Jun 05 '24 19:06 cameronmeissner

The RP change has already rolled out to all regions, now we can merge this, @cameronmeissner could you help to review again?

do we still want/need to merge this?

Hi @cameronmeissner yes it's still needed.

gaopenghigh avatar Jul 17 '24 01:07 gaopenghigh