✨ VNF-3903 Add rollback and change plan support to load balancer actions - #142
✨ VNF-3903 Add rollback and change plan support to load balancer actions#142KobayashiNozomi wants to merge 5 commits into
Conversation
yamashita-hiroto
left a comment
yamashita-hiroto
left a comment
There was a problem hiding this comment.
LGTM
コミット毎に分けて下記対応を実施
- chage-plan, is_rollback_allowed の追加
- gofmt によるフォーマット修正
- ecl_mlb_rule_v1 の conditions フィールドにおける型の不一致 に対する注意コメント追加
There was a problem hiding this comment.
VNF-3903 のレビューです。
Day 5 の API 差分は mVNA API リファレンスの x-release: 5 と突き合わせ、対応漏れが無いことを確認しています。
[MUST] change-plan の後に Load Balancer が作り替えられる
ecl/resource_ecl_mlb_load_balancer_v1.go の plan_id は Required と ForceNew の組み合わせです。
resourceMLBLoadBalancerV1Read は d.Set("plan_id", loadBalancer.PlanID) で実機の値を state に書き戻します。
この 2 つにより、change-plan アクションで実機の plan_id が変わると、次の refresh がそれを ForceNew の差分として検出し、Load Balancer が destroy と create をたどります。
VNF-3903 の Lab 検証ログにこの挙動が残っています。
~ plan_id = "b24df6f8-..." -> "19cbbf78-..." # forces replacement
...
ecl_mlb_load_balancer_v1.test: Destruction complete after 2m15s
作り替えは 1 回では収まりません。
config の plan_id が A、change_plan が B のとき、apply ごとに次の循環が起きます。
apply 1: LB を A で作成 → change-plan で実機が B に (state は A)
apply 2: refresh で実機 B を検出 → ForceNew で LB を destroy + create (A に戻る)
→ load_balancer_id が変わるので action も再作成 → change-plan で再び B に
apply 3: 以降 apply 2 と同じ
この循環を避けられる config は、plan_id と change_plan に同じ値を書く場合だけです。
ただしその場合は既存 Load Balancer のプラン変更が起きないため、設定を維持したままプランを変更するという Day 5 の目的を満たせません。
修正方法
plan_id から ForceNew を外すと収束します。
"plan_id": &schema.Schema{
Type: schema.TypeString,
Required: true,
- ForceNew: true,
},resourceMLBLoadBalancerV1UpdateAttributes が HasChange を見るのは name と description と tags だけで、resourceMLBLoadBalancerV1UpdateConfigurations が見るのは syslog_servers と interfaces だけです。
そのため plan_id のみが変わった Update は API を呼ばない no-op になります。
プラン変更は ecl_mlb_load_balancer_action_v1 の change_plan が担う形になり、各リソースで staged 設定を作ってアクションリソースで apply_configurations を実行する既存の設計とも揃います。
他のコード変更は不要です。
修正後、plan_id と change_plan の両方に B を指定すると次のように収束します。
apply 1: LB は update in-place (no-op) → Read で state は実機値 A のまま
→ action が change-plan を実行 → 実機が B に
plan 2: refresh で実機 B を読んで state が B に → config B と一致で差分なし
action も CheckChangePlanRequired で実機 B == B のためスキップ
apply 1 の終了時点では state の A と plan した B がずれるため、Provider produced inconsistent result after apply がログに出ます。
helper/schema は legacy_type_system を立てるので、これはエラーではなく警告に留まります。
動作確認
configuration_status が CREATE_STAGED のままだと validation で弾かれるため、先に apply_configurations を完了させてください。
plan_id = Aとapply_configurations = trueで apply し、configuration_statusがACTIVEになるまで待つplan_idを B に変更し、あわせてchange_plan = Bを指定してterraform planを実行する- apply して change-plan が動き、destroy のログが出ないことを確認する
- 再度
terraform planを実行し、plan_idの差分が出ないことを確認する - rollback は
system_updateを適用してからrollback = "true"を指定して apply し、revisionがcurrent_revisionに戻ることを確認する
2 で -/+ destroy and then create replacement ではなく ~ update in-place になること が今回の修正の証跡です。
プラン変更でインターフェース数が変わると interfaces の MinItems と MaxItems に引っかかります。
Lab 検証と同じく、インターフェース数が等しいプラン同士 (50M_HA_4IF から 200M_HA_4IF) を選んでください。
ドキュメントへの追記
website/docs/r/mlb_load_balancer_action_v1.html.markdown の change_plan の説明に、次の 2 点を注意書きとして追記してください。
change_planでプランを変更したときはecl_mlb_load_balancer_v1側のplan_idも同じ値に更新する- 更新しない場合、次回の
terraform planでplan_idの差分として検出される
[IMO] system_update の rollback の typo を検出する
system_update は schema.TypeMap に Elem: &schema.Resource{...} を与えた形です。
legacy SDK は TypeMap の Elem に渡した *schema.Resource を無視するため、任意のキーと文字列値が通ります。
その結果、typo したキーによって失敗の仕方が分かれます。
system_update_id：systemUpdateMap["system_update_id"].(string)が nil への型アサーションで panic するため気付けるrollback：if v, ok := systemUpdateMap["rollback"]; ok && v.(string) != ""が偽になってrollback = falseとなり、ロールバックのつもりでシステムアップデートが走る
本筋は TypeList と MaxItems: 1 への変更です。
ただし HCL の書き方が system_update = { ... } から system_update { ... } に変わり、state migration も必要になります。
既存ユーザーの config が壊れるため、この変更は選びにくいと考えます。
schema を変えずに済む緩和策として、ValidateFunc で未知のキーを弾く方法があります。
vendor 済みの helper/schema の validateMap はマップ全体を ValidateFunc に渡すので、TypeMap でも機能します。
"system_update": &schema.Schema{
Type: schema.TypeMap,
Optional: true,
Elem: &schema.Resource{ /* 既存のまま */ },
ValidateFunc: func(v interface{}, k string) ([]string, []error) {
var errs []error
for key := range v.(map[string]interface{}) {
if key != "system_update_id" && key != "rollback" {
errs = append(errs, fmt.Errorf("%s: unsupported key %q", k, key))
}
}
return nil, errs
},
},- メリット：破壊的変更なしに typo を plan 時点で弾ける。検査するのはキー名だけなので、値が unknown でも動く
- デメリット：キーを追加するときに
ValidateFuncの更新も必要になる
[NR] certificate リソースにも同じ形が残っている
certificateCertFileSchemaForResource と certificateKeyFileSchemaForResource も TypeMap に Elem: &schema.Resource{...} を与えた形です。
ssl_key の passphrase は if passphrase, ok := file["passphrase"].(string); ok で受けており、typo すると rollback と同じく静かに無視されます。
この PR のスコープ外なので、別 Issue を切って対応するのが良いと考えます。
resource_ecl_mlb_rule_v1.go の conditions は TypeList なので、SDK 側でキー名が検証されます。
tags は任意のキーを受けるのが意図された設計です。
どちらも対象外です。
[MUST] change-plan のテストを追加する
_NoChangePlan：Load Balancer のplan_idがchange_planと一致し、アクションを投げないケース_ApplyConfigurationsAndChangePlan：既存の_ApplyConfigurationsAndSystemUpdateに相当するケース
Read が no-op であるため、change-plan の冪等性は CheckChangePlanRequired だけが担保しています。
_NoApplyConfigurations と _NoSystemUpdate と同じ粒度で押さえてください。
[IMO] ドキュメントに change_plan の Example Usage を追加する
website/docs/r/mlb_load_balancer_action_v1.html.markdown の変更は Argument Reference への追記だけです。
change_plan は新しいトップレベルの引数で、上記の注意書きのとおり ecl_mlb_load_balancer_v1 の plan_id と同じ値を書く必要があります。
両方を並べた Example Usage があると、この対応関係が伝わります。
rollback は既に例のある system_update ブロックにキーが 1 つ増えるだけなので、追記は不要と考えます。
[NITS] mock test の HCL の桁を揃える
testAccMLBV1LoadBalancerActionSystemUpdateRollback の = の位置がずれています。
Go の文字列リテラル内なので make fmtcheck では検出されません。
system_update = {
system_update_id = "31746df7-92f9-4b5e-ad05-59f6684a54eb"
- rollback = true
+ rollback = true
}3 participants
This pull request supports Managed Load Balancer Day 5.
make fmtcheckerrorReference
nttcom/eclcloud#56
Related Pull Requests
Test Results
TestMockedAccMLBV1