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

feature: make lxcfs configurable supportd in CRI #2210

Merged
merged 1 commit into from
Sep 10, 2018

Conversation

starnop
Copy link
Contributor

@starnop starnop commented Sep 6, 2018

Signed-off-by: Starnop [email protected]

Ⅰ. Describe what this PR did

Support resource review isolation via lxcfs in CRI Manager.

And define the specific naming in annotations:
LxcfsEnabled = "io.kubernetes.lxcfs.enabled"

Ⅱ. Does this pull request fix one issue?

fixes #2172

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

None.

Ⅳ. Describe how to verify it

# cat pouch.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: pouch
  labels:
    pouch: pouch
spec:
  selector:
    matchLabels:
      pouch: pouch
  template:
    metadata:
      labels:
        pouch: pouch
      annotations:
        io.kubernetes.lxcfs.enabled: "true"
    spec:
      containers:
      - name: pouch
        image: docker.io/library/busybox:latest
        command:
          - top
        resources:
          requests:
            memory: "256Mi"
          limits:
            memory: "256Mi"
# kubectl create -f pouch.yaml
# pouch ps
Name                                                                                                           ID       Status       Created       Image                                                                 Runtime
k8s_pouch_pouch-5ddd8fc467-rmtcw_default_bc4b7972-b181-11e8-adae-42010a8c0003_0                                5391a9   Up 8 hours   8 hours ago   docker.io/library/busybox:latest                                      runc
k8s_POD_pouch-5ddd8fc467-rmtcw_default_bc4b7972-b181-11e8-adae-42010a8c0003_0                                  60a833   Up 8 hours   8 hours ago   registry.cn-hangzhou.aliyuncs.com/google-containers/pause-amd64:3.0   runc
# pouch exec k8s_pouch_pouch-5ddd8fc467-rmtcw_default_bc4b7972-b181-11e8-adae-42010a8c0003_0 cat /proc/meminfo 
MemTotal:         262144 kB
MemFree:          261368 kB
MemAvailable:     261368 kB
Buffers:               0 kB
Cached:                0 kB
SwapCached:            0 kB
Active:              156 kB
Inactive:              0 kB
Active(anon):        156 kB
Inactive(anon):        0 kB
Active(file):          0 kB
Inactive(file):        0 kB
Unevictable:           0 kB
Mlocked:            3652 kB
SwapTotal:             0 kB
SwapFree:              0 kB
Dirty:               292 kB
Writeback:             0 kB
AnonPages:        561860 kB
Mapped:           395920 kB
Shmem:              5640 kB
Slab:               0 kB
SReclaimable:          0 kB
SUnreclaim:            0 kB
KernelStack:        6112 kB
PageTables:         6760 kB
NFS_Unstable:          0 kB
Bounce:                0 kB
WritebackTmp:          0 kB
CommitLimit:     1890884 kB
Committed_AS:    1550596 kB
VmallocTotal:   34359738367 kB
VmallocUsed:           0 kB
VmallocChunk:          0 kB
HardwareCorrupted:     0 kB
AnonHugePages:         0 kB
ShmemHugePages:        0 kB
ShmemPmdMapped:        0 kB
CmaTotal:              0 kB
CmaFree:               0 kB
HugePages_Total:       0
HugePages_Free:        0
HugePages_Rsvd:        0
HugePages_Surp:        0
Hugepagesize:       2048 kB
DirectMap4k:       88012 kB
DirectMap2M:     3844096 kB
DirectMap1G:           0 kB

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Sep 6, 2018

Codecov Report

Merging #2210 into master will decrease coverage by 0.11%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2210      +/-   ##
==========================================
- Coverage    64.9%   64.79%   -0.12%     
==========================================
  Files         208      208              
  Lines       16722    16728       +6     
==========================================
- Hits        10854    10839      -15     
- Misses       4519     4531      +12     
- Partials     1349     1358       +9
Flag Coverage Δ
#criv1alpha1test 32.81% <0%> (-0.21%) ⬇️
#criv1alpha2test 33.47% <42.85%> (-0.24%) ⬇️
#integrationtest 39.98% <0%> (+0.01%) ⬆️
#unittest 23.83% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
cri/v1alpha2/cri_types.go 100% <ø> (ø) ⬆️
cri/v1alpha2/cri_utils.go 82.58% <100%> (+0.02%) ⬆️
cri/v1alpha2/cri.go 62.14% <33.33%> (-1.15%) ⬇️
ctrd/container.go 52.87% <0%> (-0.96%) ⬇️
cri/v1alpha1/cri.go 61.88% <0%> (-0.68%) ⬇️
daemon/mgr/container.go 56.14% <0%> (-0.62%) ⬇️
daemon/logger/jsonfile/utils.go 73.17% <0%> (+1.62%) ⬆️

@allencloud
Copy link
Collaborator

Do we need to cover this in the CRI changelog of Documentation? @starnop
If that, please finish the doc part along with this pull request.

@@ -273,11 +274,14 @@ func (c *CriManager) RunPodSandbox(ctx context.Context, r *runtime.RunPodSandbox
}
}

enableLxcfs, _ := strconv.ParseBool(config.Annotations[anno.LxcfsEnabled])
Copy link
Collaborator

Choose a reason for hiding this comment

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

What will happen if config.Annotations[anno.LxcfsEnabled] is invalid and the parsing returns a non-nil error? @starnop

@@ -17,6 +17,9 @@ type SandboxMeta struct {

// Runtime is the runtime of sandbox
Runtime string

// Runtime whether to enable lxcfs for a container
LxcfsEnabled bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about v1alpha1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allencloud Support for CRI functionality extensions is only available for v1alpha2, WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I am OK on it. We need to tell all these details to the users. About the change, about incompatibility, or a plan when to abandon v1apha1.

@starnop
Copy link
Contributor Author

starnop commented Sep 7, 2018

With respect to CRI's support for the annotation field, I thought it would be nice to create a new document to record these changes.

@allencloud
Copy link
Collaborator

With respect to CRI's support for the annotation field, I thought it would be nice to create a new document to record these changes.

It could not be better if we do this.

Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM @starnop we can add test if we introduce the end -2-end test #2198
ping @YaoZengzeng

@YaoZengzeng
Copy link
Contributor

LGTM

@fuweid fuweid merged commit 7ef7dd6 into AliyunContainerService:master Sep 10, 2018
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.

[feature request] make lxcfs configurable supportd in CRI managers for Kubernetes
6 participants