Skip to content

Commit 01a90a6

Browse files
allencloudallencloud
authored
Merge pull request #1041 from HusterWan/zr/port-validate
refactor: mv cli params parse and validate part of code into opts package
2 parents 28f5fa1 + 6257735 commit 01a90a6

33 files changed

+1402
-864
lines changed

‎cli/container.go‎

Lines changed: 34 additions & 297 deletions
Large diffs are not rendered by default.

‎cli/container_test.go‎

Lines changed: 0 additions & 464 deletions
This file was deleted.

‎cli/update.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55

66
"github.com/alibaba/pouch/apis/types"
7+
"github.com/alibaba/pouch/pkg/opts"
78
"github.com/alibaba/pouch/pkg/reference"
89

910
"github.com/spf13/cobra"
@@ -58,21 +59,21 @@ func (uc *UpdateCommand) updateRun(args []string) error {
5859
container := args[0]
5960
ctx := context.Background()
6061

61-
labels, err := parseLabels(uc.labels)
62+
labels, err := opts.ParseLabels(uc.labels)
6263
if err != nil {
6364
return err
6465
}
6566

66-
if err := validateMemorySwappiness(uc.memorySwappiness); err != nil {
67+
if err := opts.ValidateMemorySwappiness(uc.memorySwappiness); err != nil {
6768
return err
6869
}
6970

70-
memory, err := parseMemory(uc.memory)
71+
memory, err := opts.ParseMemory(uc.memory)
7172
if err != nil {
7273
return err
7374
}
7475

75-
memorySwap, err := parseMemorySwap(uc.memorySwap)
76+
memorySwap, err := opts.ParseMemorySwap(uc.memorySwap)
7677
if err != nil {
7778
return err
7879
}
@@ -87,7 +88,7 @@ func (uc *UpdateCommand) updateRun(args []string) error {
8788
BlkioWeight: uc.blkioWeight,
8889
}
8990

90-
restartPolicy, err := parseRestartPolicy(uc.restartPolicy)
91+
restartPolicy, err := opts.ParseRestartPolicy(uc.restartPolicy)
9192
if err != nil {
9293
return err
9394
}

‎daemon/mgr/container_validation.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
package mgr
2+
3+
import (
4+
"github.com/alibaba/pouch/apis/types"
5+
)
6+
7+
// verifyContainerSetting is to verify the correctness of hostconfig and config.
8+
func (mgr *ContainerManager) verifyContainerSetting(hostConfig *types.HostConfig, config *types.ContainerConfig) error {
9+
if config != nil {
10+
// TODO
11+
}
12+
return nil
13+
}

‎daemon/mgr/spec_devices.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import (
77
"path/filepath"
88
"strings"
99

10-
"github.com/alibaba/pouch/pkg/runconfig"
10+
"github.com/alibaba/pouch/pkg/opts"
1111

1212
"github.com/opencontainers/runc/libcontainer/configs"
1313
"github.com/opencontainers/runc/libcontainer/devices"
@@ -109,7 +109,7 @@ func setupDevices(ctx context.Context, meta *ContainerMeta, spec *SpecWrapper) e
109109
}
110110
} else {
111111
for _, deviceMapping := range meta.HostConfig.Devices {
112-
if !runconfig.ValidDeviceMode(deviceMapping.CgroupPermissions) {
112+
if !opts.ValidateDeviceMode(deviceMapping.CgroupPermissions) {
113113
return fmt.Errorf("%s invalid device mode: %s", deviceMapping.PathOnHost, deviceMapping.CgroupPermissions)
114114
}
115115
d, dPermissions, err := devicesFromPath(deviceMapping.PathOnHost, deviceMapping.PathInContainer, deviceMapping.CgroupPermissions)

‎pkg/runconfig/hostconfig.go‎ renamed to ‎pkg/opts/devicemappings.go‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
package runconfig
1+
package opts
22

33
import (
44
"fmt"
@@ -7,8 +7,27 @@ import (
77
"github.com/alibaba/pouch/apis/types"
88
)
99

10-
// ParseDevice parses a device mapping string to a container.DeviceMapping struct
11-
func ParseDevice(device string) (*types.DeviceMapping, error) {
10+
// ParseDeviceMappings parse devicemappings
11+
func ParseDeviceMappings(devices []string) ([]*types.DeviceMapping, error) {
12+
results := []*types.DeviceMapping{}
13+
for _, device := range devices {
14+
deviceMapping, err := parseDevice(device)
15+
if err != nil {
16+
return nil, fmt.Errorf("failed to parse devices: %v", err)
17+
}
18+
19+
if !ValidateDeviceMode(deviceMapping.CgroupPermissions) {
20+
return nil, fmt.Errorf("%s invalid device mode: %s", device, deviceMapping.CgroupPermissions)
21+
}
22+
23+
results = append(results, deviceMapping)
24+
}
25+
return results, nil
26+
27+
}
28+
29+
// parseDevice parses a device mapping string to a container.DeviceMapping struct
30+
func parseDevice(device string) (*types.DeviceMapping, error) {
1231
src := ""
1332
dst := ""
1433
permissions := "rwm"
@@ -38,9 +57,9 @@ func ParseDevice(device string) (*types.DeviceMapping, error) {
3857
return deviceMapping, nil
3958
}
4059

41-
// ValidDeviceMode checks if the mode for device is valid or not.
60+
// ValidateDeviceMode checks if the mode for device is valid or not.
4261
// valid mode is a composition of r (read), w (write), and m (mknod).
43-
func ValidDeviceMode(mode string) bool {
62+
func ValidateDeviceMode(mode string) bool {
4463
var legalDeviceMode = map[rune]bool{
4564
'r': true,
4665
'w': true,

‎pkg/opts/devicemappings_test.go‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
package opts
2+
3+
import (
4+
"fmt"
5+
"reflect"
6+
"testing"
7+
8+
"github.com/alibaba/pouch/apis/types"
9+
10+
"github.com/stretchr/testify/assert"
11+
)
12+
13+
func TestParseDeviceMapping(t *testing.T) {
14+
type args struct {
15+
device []string
16+
}
17+
tests := []struct {
18+
name string
19+
args args
20+
want []*types.DeviceMapping
21+
wantErr bool
22+
}{
23+
{
24+
name: "deviceMapping1",
25+
args: args{
26+
device: []string{"/dev/deviceName:/dev/deviceName:mrw"},
27+
},
28+
want: []*types.DeviceMapping{{
29+
PathOnHost: "/dev/deviceName",
30+
PathInContainer: "/dev/deviceName",
31+
CgroupPermissions: "mrw",
32+
}},
33+
34+
wantErr: false,
35+
},
36+
{
37+
name: "deviceMapping2",
38+
args: args{
39+
device: []string{"/dev/deviceName:"},
40+
},
41+
want: []*types.DeviceMapping{{
42+
PathOnHost: "/dev/deviceName",
43+
PathInContainer: "/dev/deviceName",
44+
CgroupPermissions: "rwm",
45+
}},
46+
wantErr: false,
47+
},
48+
{
49+
name: "deviceMappingWrong1",
50+
args: args{
51+
device: []string{"/dev/deviceName:/dev/deviceName:rrw"},
52+
},
53+
wantErr: true,
54+
},
55+
{
56+
name: "deviceMappingWrong2",
57+
args: args{
58+
device: []string{"/dev/deviceName:/dev/deviceName:arw"},
59+
},
60+
wantErr: true,
61+
},
62+
{
63+
name: "deviceMappingWrong3",
64+
args: args{
65+
device: []string{"/dev/deviceName:/dev/deviceName:mrw:mrw"},
66+
},
67+
wantErr: true,
68+
},
69+
}
70+
for _, tt := range tests {
71+
t.Run(tt.name, func(t *testing.T) {
72+
got, err := ParseDeviceMappings(tt.args.device)
73+
if (err != nil) != tt.wantErr {
74+
t.Errorf("parseDeviceMappings() error = %v, wantErr %v", err, tt.wantErr)
75+
return
76+
}
77+
if !reflect.DeepEqual(got, tt.want) {
78+
t.Errorf("parseDeviceMappings() = %v, want %v", got, tt.want)
79+
}
80+
})
81+
}
82+
}
83+
84+
type tDeviceModeCase struct {
85+
input string
86+
expected bool
87+
}
88+
89+
type tParseDeviceCase struct {
90+
input string
91+
expected *types.DeviceMapping
92+
err error
93+
}
94+
95+
func Test_parseDevice(t *testing.T) {
96+
for _, tc := range []tParseDeviceCase{
97+
{
98+
input: "/dev/zero:/dev/zero:rwm",
99+
expected: &types.DeviceMapping{
100+
PathOnHost: "/dev/zero",
101+
PathInContainer: "/dev/zero",
102+
CgroupPermissions: "rwm",
103+
},
104+
err: nil,
105+
}, {
106+
input: "/dev/zero:rwm",
107+
expected: &types.DeviceMapping{
108+
PathOnHost: "/dev/zero",
109+
PathInContainer: "rwm",
110+
CgroupPermissions: "rwm",
111+
},
112+
err: nil,
113+
}, {
114+
input: "/dev/zero",
115+
expected: &types.DeviceMapping{
116+
PathOnHost: "/dev/zero",
117+
PathInContainer: "/dev/zero",
118+
CgroupPermissions: "rwm",
119+
},
120+
err: nil,
121+
}, {
122+
input: "/dev/zero:/dev/testzero:rwm",
123+
expected: &types.DeviceMapping{
124+
PathOnHost: "/dev/zero",
125+
PathInContainer: "/dev/testzero",
126+
CgroupPermissions: "rwm",
127+
},
128+
err: nil,
129+
}, {
130+
input: "/dev/zero:/dev/testzero:rwm:tooLong",
131+
expected: nil,
132+
err: fmt.Errorf("invalid device specification: /dev/zero:/dev/testzero:rwm:tooLong"),
133+
},
134+
} {
135+
output, err := parseDevice(tc.input)
136+
assert.Equal(t, tc.err, err, tc.input)
137+
assert.Equal(t, tc.expected, output, tc.input)
138+
}
139+
}
140+
141+
func TestValidateDeviceMode(t *testing.T) {
142+
for _, modeCase := range []tDeviceModeCase{
143+
{
144+
input: "rwm",
145+
expected: true,
146+
}, {
147+
input: "r",
148+
expected: true,
149+
}, {
150+
input: "rw",
151+
expected: true,
152+
}, {
153+
input: "rr",
154+
expected: false,
155+
}, {
156+
input: "rxm",
157+
expected: false,
158+
},
159+
} {
160+
isValid := ValidateDeviceMode(modeCase.input)
161+
assert.Equal(t, modeCase.expected, isValid, modeCase.input)
162+
}
163+
}

‎pkg/opts/diskquota.go‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
package opts
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
)
7+
8+
// ParseDiskQuota parses diskquota configurations of container.
9+
func ParseDiskQuota(quotas []string) (map[string]string, error) {
10+
var quotaMaps = make(map[string]string)
11+
12+
for _, quota := range quotas {
13+
if quota == "" {
14+
return nil, fmt.Errorf("invalid format for disk quota: %s", quota)
15+
}
16+
17+
parts := strings.Split(quota, "=")
18+
switch len(parts) {
19+
case 1:
20+
quotaMaps["/"] = parts[0]
21+
case 2:
22+
quotaMaps[parts[0]] = parts[1]
23+
default:
24+
return nil, fmt.Errorf("invalid format for disk quota: %s", quota)
25+
}
26+
}
27+
28+
return quotaMaps, nil
29+
}
30+
31+
// ValidateDiskQuota verifies diskquota configurations of container.
32+
func ValidateDiskQuota(quotaMaps map[string]string) error {
33+
// TODO
34+
return nil
35+
}

‎pkg/opts/diskquota_test.go‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
package opts
2+
3+
import (
4+
"reflect"
5+
"testing"
6+
)
7+
8+
func TestParseDiskQuota(t *testing.T) {
9+
type args struct {
10+
diskquota []string
11+
}
12+
tests := []struct {
13+
name string
14+
args args
15+
want map[string]string
16+
wantErr bool
17+
}{
18+
// TODO: Add test cases.
19+
{name: "test1", args: args{diskquota: []string{""}}, want: nil, wantErr: true},
20+
{name: "test2", args: args{diskquota: []string{"foo=foo=foo"}}, want: nil, wantErr: true},
21+
{name: "test3", args: args{diskquota: []string{"foo"}}, want: map[string]string{"/": "foo"}, wantErr: false},
22+
{name: "test3", args: args{diskquota: []string{"foo=foo"}}, want: map[string]string{"foo": "foo"}, wantErr: false},
23+
}
24+
for _, tt := range tests {
25+
t.Run(tt.name, func(t *testing.T) {
26+
got, err := ParseDiskQuota(tt.args.diskquota)
27+
if (err != nil) != tt.wantErr {
28+
t.Errorf("ParseDiskQuota() error = %v, wantErr %v", err, tt.wantErr)
29+
return
30+
}
31+
if !reflect.DeepEqual(got, tt.want) {
32+
t.Errorf("ParseDiskQuota() = %v, want %v", got, tt.want)
33+
}
34+
})
35+
}
36+
}

‎pkg/opts/intel_rdt.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
package opts
2+
3+
// ParseIntelRdt parses inter-rdt params of container
4+
func ParseIntelRdt(intelRdtL3Cbm string) (string, error) {
5+
// FIXME: add Intel RDT L3 Cbm validation
6+
return intelRdtL3Cbm, nil
7+
}
8+
9+
// TODO: ValidateInterRdt

0 commit comments

Comments
 (0)