Skip to content

Commit 2070dfa

Browse files
dtantsurdtantsur
committed
Fix configdrive handling
I've spotted a few errors with the configdrive handling: * We forgot to add configdrive to 'deploy failed' handler, so a repeated deployment will always happen without a configdrive. * ConfigDrive creation is conditional on UserData presence. This is incorrect, any field being populated should trigger configdrive. * There was no check on empty networkData, so we always tried to deserialize empty string with YAML (which works, but is incorrect). Create a new helper for configdrive, fix inline comments and cover configdrive creation with unit tests.
1 parent f5228fa commit 2070dfa

File tree

2 files changed

+201
-49
lines changed

2 files changed

+201
-49
lines changed

‎pkg/provisioner/ironic/configdrive_test.go

+134
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
package ironic
2+
3+
import (
4+
"testing"
5+
6+
"github.com/gophercloud/gophercloud/openstack/baremetal/v1/nodes"
7+
"github.com/gophercloud/gophercloud/openstack/baremetalintrospection/v1/introspection"
8+
"github.com/stretchr/testify/assert"
9+
10+
"github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
11+
"github.com/metal3-io/baremetal-operator/pkg/bmc"
12+
"github.com/metal3-io/baremetal-operator/pkg/provisioner"
13+
"github.com/metal3-io/baremetal-operator/pkg/provisioner/fixture"
14+
"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/clients"
15+
"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic/testserver"
16+
)
17+
18+
func TestEmpty(t *testing.T) {
19+
nodeUUID := "33ce8659-7400-4c68-9535-d10766f07a58"
20+
21+
cases := []struct {
22+
name string
23+
hostData provisioner.HostConfigData
24+
expected nodes.ConfigDrive
25+
}{
26+
{
27+
name: "empty",
28+
hostData: fixture.NewHostConfigData("", "", ""),
29+
expected: nodes.ConfigDrive{},
30+
},
31+
{
32+
name: "everything",
33+
hostData: fixture.NewHostConfigData("testUserData", "test: NetworkData", "test: Meta"),
34+
expected: nodes.ConfigDrive{
35+
MetaData: map[string]interface{}{
36+
"local-hostname": "myhost",
37+
"local_hostname": "myhost",
38+
"metal3-name": "myhost",
39+
"metal3-namespace": "myns",
40+
"name": "myhost",
41+
"test": "Meta",
42+
},
43+
NetworkData: map[string]interface{}{
44+
"test": "NetworkData",
45+
},
46+
UserData: "testUserData",
47+
},
48+
},
49+
{
50+
name: "only network data",
51+
hostData: fixture.NewHostConfigData("", "test: NetworkData", ""),
52+
expected: nodes.ConfigDrive{
53+
MetaData: map[string]interface{}{
54+
"local-hostname": "myhost",
55+
"local_hostname": "myhost",
56+
"metal3-name": "myhost",
57+
"metal3-namespace": "myns",
58+
"name": "myhost",
59+
},
60+
NetworkData: map[string]interface{}{
61+
"test": "NetworkData",
62+
},
63+
},
64+
},
65+
{
66+
name: "only user data",
67+
hostData: fixture.NewHostConfigData("testUserData", "", ""),
68+
expected: nodes.ConfigDrive{
69+
MetaData: map[string]interface{}{
70+
"local-hostname": "myhost",
71+
"local_hostname": "myhost",
72+
"metal3-name": "myhost",
73+
"metal3-namespace": "myns",
74+
"name": "myhost",
75+
},
76+
UserData: "testUserData",
77+
},
78+
},
79+
{
80+
name: "only meta data",
81+
hostData: fixture.NewHostConfigData("", "", "test: Meta"),
82+
expected: nodes.ConfigDrive{
83+
MetaData: map[string]interface{}{
84+
"local-hostname": "myhost",
85+
"local_hostname": "myhost",
86+
"metal3-name": "myhost",
87+
"metal3-namespace": "myns",
88+
"name": "myhost",
89+
"test": "Meta",
90+
},
91+
},
92+
},
93+
}
94+
95+
for _, tc := range cases {
96+
t.Run(tc.name, func(t *testing.T) {
97+
ironic := testserver.NewIronic(t).Ready().Node(nodes.Node{
98+
ProvisionState: string(nodes.Active),
99+
UUID: nodeUUID,
100+
})
101+
ironic.Start()
102+
defer ironic.Stop()
103+
104+
inspector := testserver.NewInspector(t).Ready().WithIntrospection(nodeUUID, introspection.Introspection{
105+
Finished: false,
106+
})
107+
inspector.Start()
108+
defer inspector.Stop()
109+
110+
host := makeHost()
111+
host.Status.Provisioning.ID = nodeUUID
112+
publisher := func(reason, message string) {}
113+
auth := clients.AuthConfig{Type: clients.NoAuth}
114+
prov, err := newProvisionerWithSettings(host, bmc.Credentials{}, publisher,
115+
ironic.Endpoint(), auth, inspector.Endpoint(), auth,
116+
)
117+
if err != nil {
118+
t.Fatalf("could not create provisioner: %s", err)
119+
}
120+
121+
result, err := prov.getConfigDrive(provisioner.ProvisionData{
122+
HostConfig: tc.hostData,
123+
BootMode: v1alpha1.DefaultBootMode,
124+
})
125+
126+
if len(tc.expected.MetaData) > 0 {
127+
tc.expected.MetaData["uuid"] = string(prov.objectMeta.UID)
128+
}
129+
130+
assert.Equal(t, tc.expected, result)
131+
assert.NoError(t, err)
132+
})
133+
}
134+
}

‎pkg/provisioner/ironic/ironic.go

+67-49
Original file line numberDiff line numberDiff line change
@@ -1239,6 +1239,59 @@ func (p *ironicProvisioner) Prepare(data provisioner.PrepareData, unprepared boo
12391239
return
12401240
}
12411241

1242+
func (p *ironicProvisioner) getConfigDrive(data provisioner.ProvisionData) (configDrive nodes.ConfigDrive, err error) {
1243+
// Retrieve instance specific user data (cloud-init, ignition, etc).
1244+
userData, err := data.HostConfig.UserData()
1245+
if err != nil {
1246+
return configDrive, errors.Wrap(err, "could not retrieve user data")
1247+
}
1248+
if userData != "" {
1249+
configDrive.UserData = userData
1250+
}
1251+
1252+
// Retrieve OpenStack network_data. Default value is empty.
1253+
networkDataRaw, err := data.HostConfig.NetworkData()
1254+
if err != nil {
1255+
return configDrive, errors.Wrap(err, "could not retrieve network data")
1256+
}
1257+
if networkDataRaw != "" {
1258+
var networkData map[string]interface{}
1259+
if err = yaml.Unmarshal([]byte(networkDataRaw), &networkData); err != nil {
1260+
return configDrive, errors.Wrap(err, "failed to unmarshal network_data.json from secret")
1261+
}
1262+
configDrive.NetworkData = networkData
1263+
}
1264+
1265+
// Retrieve meta data with fallback to defaults from provisioner.
1266+
metaData := map[string]interface{}{
1267+
"uuid": string(p.objectMeta.UID),
1268+
"metal3-namespace": p.objectMeta.Namespace,
1269+
"metal3-name": p.objectMeta.Name,
1270+
"local-hostname": p.objectMeta.Name,
1271+
"local_hostname": p.objectMeta.Name,
1272+
"name": p.objectMeta.Name,
1273+
}
1274+
metaDataRaw, err := data.HostConfig.MetaData()
1275+
if err != nil {
1276+
return configDrive, errors.Wrap(err, "could not retrieve metadata")
1277+
}
1278+
if metaDataRaw != "" {
1279+
if err = yaml.Unmarshal([]byte(metaDataRaw), &metaData); err != nil {
1280+
return configDrive, errors.Wrap(err, "failed to unmarshal metadata from secret")
1281+
}
1282+
}
1283+
1284+
// Set metaData if any field is populated by a user.
1285+
if metaDataRaw != "" || networkDataRaw != "" || userData != "" {
1286+
configDrive.MetaData = metaData
1287+
p.log.Info("triggering provisioning with config drive")
1288+
} else {
1289+
p.log.Info("triggering provisioning without config drive")
1290+
}
1291+
1292+
return
1293+
}
1294+
12421295
// Provision writes the image from the host spec to the host. It may
12431296
// be called multiple times, and should return true for its dirty flag
12441297
// until the deprovisioning operation is completed.
@@ -1277,8 +1330,18 @@ func (p *ironicProvisioner) Provision(data provisioner.ProvisionData) (result pr
12771330
return provResult, err
12781331
}
12791332

1280-
return p.changeNodeProvisionState(ironicNode,
1281-
nodes.ProvisionStateOpts{Target: nodes.TargetActive})
1333+
configDrive, err := p.getConfigDrive(data)
1334+
if err != nil {
1335+
return transientError(err)
1336+
}
1337+
1338+
return p.changeNodeProvisionState(
1339+
ironicNode,
1340+
nodes.ProvisionStateOpts{
1341+
Target: nodes.TargetActive,
1342+
ConfigDrive: configDrive,
1343+
},
1344+
)
12821345

12831346
case nodes.Manageable:
12841347
return p.changeNodeProvisionState(ironicNode,
@@ -1303,54 +1366,9 @@ func (p *ironicProvisioner) Provision(data provisioner.ProvisionData) (result pr
13031366
// setting the state to "active".
13041367
p.log.Info("making host active")
13051368

1306-
// Retrieve cloud-init user data
1307-
userData, err := data.HostConfig.UserData()
1308-
if err != nil {
1309-
return transientError(errors.Wrap(err, "could not retrieve user data"))
1310-
}
1311-
1312-
// Retrieve cloud-init network_data.json. Default value is empty
1313-
networkDataRaw, err := data.HostConfig.NetworkData()
1314-
if err != nil {
1315-
return transientError(errors.Wrap(err, "could not retrieve network data"))
1316-
}
1317-
var networkData map[string]interface{}
1318-
if err = yaml.Unmarshal([]byte(networkDataRaw), &networkData); err != nil {
1319-
return transientError(errors.Wrap(err, "failed to unmarshal network_data.json from secret"))
1320-
}
1321-
1322-
// Retrieve cloud-init meta_data.json with falback to default
1323-
metaData := map[string]interface{}{
1324-
"uuid": string(p.objectMeta.UID),
1325-
"metal3-namespace": p.objectMeta.Namespace,
1326-
"metal3-name": p.objectMeta.Name,
1327-
"local-hostname": p.objectMeta.Name,
1328-
"local_hostname": p.objectMeta.Name,
1329-
"name": p.objectMeta.Name,
1330-
}
1331-
metaDataRaw, err := data.HostConfig.MetaData()
1369+
configDrive, err := p.getConfigDrive(data)
13321370
if err != nil {
1333-
return transientError(errors.Wrap(err, "could not retrieve metadata"))
1334-
}
1335-
if metaDataRaw != "" {
1336-
if err = yaml.Unmarshal([]byte(metaDataRaw), &metaData); err != nil {
1337-
return transientError(errors.Wrap(err, "failed to unmarshal metadata from secret"))
1338-
}
1339-
}
1340-
1341-
var configDrive nodes.ConfigDrive
1342-
if userData != "" {
1343-
configDrive = nodes.ConfigDrive{
1344-
UserData: userData,
1345-
MetaData: metaData,
1346-
NetworkData: networkData,
1347-
}
1348-
if err != nil {
1349-
return transientError(errors.Wrap(err, "failed to build config drive"))
1350-
}
1351-
p.log.Info("triggering provisioning with config drive")
1352-
} else {
1353-
p.log.Info("triggering provisioning without config drive")
1371+
return transientError(err)
13541372
}
13551373

13561374
return p.changeNodeProvisionState(

0 commit comments

Comments
 (0)