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

Optimize k8s runtime #1600

Merged
merged 4 commits into from
Jul 27, 2022
Merged

Conversation

kakaZhou719
Copy link
Member

@kakaZhou719 kakaZhou719 commented Jul 22, 2022

Describe what this PR does / why we need it

optimize k8s runtime

  1. rename k8s runtime struct name from "KubeadmRuntime" to "Runtime"
  2. move out registry related configs
  3. move out kubeadm related configs

this pr only move out some code section nothing to do with the logic.

the next steps:

Redesign the runtime interface, make this module more clearly.

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

kakzhou719 added 3 commits July 21, 2022 17:23

func (c *Config) GenerateHtPasswd() (string, error) {
if c.Username == "" || c.Password == "" {
return "", fmt.Errorf("failed to generate htpasswd: registry username or passwodr is empty")
Copy link
Member

Choose a reason for hiding this comment

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

passwodr => password

Copy link
Member

Choose a reason for hiding this comment

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

Ht?

Copy link
Member Author

Choose a reason for hiding this comment

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

i think this func means to gen HTTP basic authentication with username and password. so it could be rename to "GenerateHttpBasicAuth"

return fmt.Sprintf("%s:%s", c.Domain, c.Port)
}

func GetConfig(rootfs string, defaultRegistryIP net.IP) *Config {
Copy link
Member

Choose a reason for hiding this comment

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

defaultRegistryIP => registryIP

}
registryConfigPath := filepath.Join(rootfs, common.EtcDir, ConfigFile)
if !osi.IsFileExist(registryConfigPath) {
logrus.Debug("use default registry config")
Copy link
Member

Choose a reason for hiding this comment

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

default registry configuration is used: \n %+v, DefaultConfig


func GetConfig(rootfs string, defaultRegistryIP net.IP) *Config {
var config Config
var DefaultConfig = &Config{
Copy link
Member

Choose a reason for hiding this comment

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

DefaultConfig => defaultConfig

if config.Domain == "" {
config.Domain = DefaultConfig.Domain
}
logrus.Debugf("show registry info, IP: %s, Domain: %s", config.IP, config.Domain)
Copy link
Member

Choose a reason for hiding this comment

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

The ultimate registry configration is:

)

var (
ContainerdShell = `if grep "SystemdCgroup = true" /etc/containerd/config.toml &> /dev/null; then
Copy link
Member

Choose a reason for hiding this comment

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

Is that possible we try not to write shell in go later?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, We have too many operations done by the shell,it is hard to read and maintain. this pr only move out some code section nothing to do with the logic. I plan to optimize them in the next steps.

@@ -12,15 +12,15 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package kubernetes
package version
Copy link
Member

Choose a reason for hiding this comment

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

"package version" makes me feel it's the version of sealer instead of "kube"

Copy link
Member Author

Choose a reason for hiding this comment

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

this package under utils, rename it to "kubeVersion" ?

Copy link
Member

Choose a reason for hiding this comment

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

Agree!

@github-actions github-actions bot added the test label Jul 27, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1600 (435710c) into main (25b39ce) will increase coverage by 2.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1600      +/-   ##
==========================================
+ Coverage   11.82%   13.98%   +2.15%     
==========================================
  Files          91       80      -11     
  Lines        8112     6859    -1253     
==========================================
  Hits          959      959              
+ Misses       7030     5777    -1253     
  Partials      123      123              
Impacted Files Coverage Δ
pkg/runtime/kubernetes/kubeadm/kubeadm_config.go 53.22% <ø> (ø)
utils/decode.go 0.00% <ø> (ø)
utils/version/version.go 68.00% <100.00%> (ø)
pkg/runtime/kubernetes/init.go
pkg/runtime/kubernetes/join_masters.go
pkg/runtime/kubernetes/join_nodes.go
pkg/runtime/kubernetes/registry.go
pkg/runtime/kubernetes/reset.go
pkg/runtime/kubernetes/runtime.go
pkg/runtime/kubernetes/update_cert.go
... and 2 more

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 25b39ce...435710c. Read the comment docs.

@allencloud
Copy link
Member

So good to see the registry related part split from runtime. @kakaZhou719 👍🏻

LGTM after updated all the change request.

Copy link
Member

@starComingup starComingup left a comment

Choose a reason for hiding this comment

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

LGTM, if you have tested it fully. It made this module more clearly.

@allencloud allencloud merged commit 33c1d18 into sealerio:main Jul 27, 2022
@kakaZhou719 kakaZhou719 deleted the optimize-k8s-runtime branch March 8, 2023 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants