Ansibleモジュールをコントリビュートしてみた話

こちらは、Ansibleアドベントカレンダーの12日目の記事です。
https://qiita.com/advent-calendar/2018/ansible

はじめに

少し真面目な記事になります。
Ansibleは、一般的にはOSレイヤーより上の構成管理の為のツールとして知られていますね。
最近ではネットワーク機器の構成管理ツールとしても語られる事が多いのですが
実はカバーしている範囲は大変多いのです。

その中で、私が注目しているのがCloudモジュールです。
AWSやGCP、AzureなどのCloudベンダーを利用する時に使う事が出来ます。
https://github.com/ansible/ansible/tree/devel/lib/ansible/modules/cloud
AWSの構成管理などでは、CloudFormationやTerraFormなどがよく挙げられるのですが
冪等性をクラウドインスタンスにも適用できたり状態の管理が必要ない、などの理由で
私はAnsibleのCloudモジュールが一番好きなのです。
これを使う事で、一般的に知られてるようなAnsibleでの管理方法と同じように
クラウンベンダーのリソースを管理することができるのです。

現状はモジュールが整備されきっていないこともあり、まだ発展途中な面もあるのですが
少しでも使ってくれる人が増えると良いなと思い
今回私が本家にコントリビュートした方法を書いておこうと思います。
具体的なモジュールを作るソースコードレベルの内容は別記事にしようと思っていますが
時系列や順序などが参考になればいいな、と思います。

OSSのコントリビュートは、慣れたら怖くないよ！

順序

前提

今回は、修正したい明らかなバグがありました。
ec2_instanceモジュールという、AWSでEC2を管理するのに利用されるプレビュー版のモジュールがあります。
現行ではec2という名前のモジュールです。
前の記事にも、少し記載しています。
https://qiita.com/morin_river/items/29f853c834f4c7cc756e

このモジュールではcheck_modeがシステム的にはサポートされていたのですが
実際に動かすとcheck_modeに関係なく実行されてしまう、という問題がありました。

プレビューモジュールだし、新しいコードが追加されているということ自体はとてもポジティブに考えています。
ですが業務上この問題を抱えたままだと使いづらいので、修正してみよう、と思いました。

ISSUEを立てる

まず、何が問題なのかを明らかにしました。
https://github.com/ansible/ansible/issues/46611

特にOSSでは、見ている人ができるだけわかりやすいISSUEにするのが大事だと考えています。
Ansibleの場合ISSUE_TEMPLATEがしっかりしているので
ある程度テンプレートにそって記載すれば内容が記載できるでしょう。

また、内容は英語で書く必要があるのですが、基本的にはGoogle翻訳にプラス程度で問題は無いと思っています。
（技術文章の翻訳は、それではとてもダメでしたが…… 宣伝 ）。
できるだけ、単純な文章から翻訳をかけ、その上でその英語を読み自分で違和感が出ないような状態を目指していました。

何がダメ で、 本来はこうあるべき というのをしっかりと記載すれば良いと思います。

そして、ISSUEを立てるとbotがよしなにラベリングしてくれます。
また、この状態でAnsibleのコミュニティIRCに入り
チャンネル内の人に「こういう問題があるんだけど、どう思う？誰も手をつけてないなら修正するよ？」的な事を言っていた気がします。
昔、AWSのNLBモジュールを作ったのですが、実は他の人が既に作業中だったという経験があったので
誰もこの問題に取り組んでいない事を確認しました。
（この作業は、本質的には不要なのでもっといい方法があればいいのですが）。
すると、当時はAnsibleのIRCでは通知機能がうまく動いていないから
修正ができたらIRCに通知してくれ、みたいな話になりました。
こういうアドバイスが聞けるのも、直接コミュニケーションの利点かな、と思います。

コードを書く

書いているコード自体は、すごくシンプルな内容です。
コードは、単純なロジックの追加をしただけです。
https://github.com/ansible/ansible/pull/46774/files#diff-2d067ab619507e90d3e575ee0d52dbb5
単純に、check_modeのパラメータがついていたら処理をスキップしているだけですね。
コードが長いのでトレースが少し大変なのですが、このあたりは慣れもあるかもしれません。
（本業のプログラマの方は、もっと長いコードでも大丈夫なのかもしれないですが）

テストコードを書く

今回、一番強調したいのはこの内容です。
レビュアーは、修正する内容のどこに問題があるかを認識しきれていないかもしれません。
特に、レビュアーがそのコードを書いた人でなければなおさらです。
そんな時に、一番コミュニケーション取りやすいのは、
テストコードを書いて変更点をわかりやすくする事だと思います。
この他人にわかりやすく伝えることが大事というのは
OSSでも普段のコミュニケーションでも変わらない事かもしれません。

また、Ansibleの場合、テストコードが追いついていない箇所もあります。
そのような場合は、テストコードからPRを出すとマージされやすくなるかもしれない、とも思います。
特に、バグに再現性がある場合はそのテストを書いて直す、という事をするのが一番の近道でしょう。

ec2_instanceモジュールでは、元々テストコードは存在していたのですが、チェックモードのテストが足りていませんでした。
なので、既存のテストコードにテストを追加し、チェックモード用に新しいテスト自体も追加しました。

既存のテストに追加した分

https://github.com/ansible/ansible/pull/46774/files#diff-6216fdd46b49fcf4fbd75b0738584021
https://github.com/ansible/ansible/pull/46774/files#diff-25bb5354720327ddc9b74a09d8f8d87c
https://github.com/ansible/ansible/pull/46774/files#diff-d3c4f3a9ae9f9ea2a57ccd90ac879a07
https://github.com/ansible/ansible/pull/46774/files#diff-dbe25295683fa8c1f9c4cab7ac4b8421
https://github.com/ansible/ansible/pull/46774/files#diff-62fa2d7358adc4e68481c9a465243402
https://github.com/ansible/ansible/pull/46774/files#diff-714d27f274a842068e1259711a66ef2d
https://github.com/ansible/ansible/pull/46774/files#diff-fe16e0c6df48c9423e6c2a8cbd76658f

新規のテスト

記事に起こすとわかりやすいのですが、書いたコードよりもテストコードの方がはるかに多くなっています。
もちろん、PythonのコードとYAMLの表現力の違いというのもあるのですが
時間もテストコードに費やした時間の方が多かったです。
ただ、他の人に説明するにはこの方法が一番の近道なはずなので
しっかりと書きたかったのですね。
このテストコードを書くにあたって、今まで知らなかったboto3ライブラリのwaiterの概念などに詳しくなれたり
テストコードを書く事でその技術の学習にもなるな、と感じました。

テストする

Ansibleのテストコードでは、統合テストとサニティーテストがあります。
それぞれ、以下のようなコマンドでテストを行う事が可能です。

test_command

$ ansible-test sanity --test pep8 ec2_instance
$ ansible-test integration ec2_instance

が、今回は統合テストが利用できませんでした。
後々、当時はAWSのテスト用パイプラインが足りていなくて
統合テスト時にAWSリソースを動かしていなかったというのがわかったのですが
当時はわかりませんでした。
なので、実行した結果を貼り、統合テストしたけど動かなかったよ
というのを明示するようにしました。

PRを作る

https://github.com/ansible/ansible/pull/46774
あとは、今までの内容をまとめて、PRを作るだけです。

• 困っていた内容
• 書いたコードの内容
• 実行して、上手く言った結果
• 実行したけどうまく行かなかった結果

冗長になりすぎない範囲で、出来るだけ多くを残しておきました。
……修正したコードはRed Hatのシニアエンジニアの人だったので、少し不安にはなったのですが
ここまでしっかり、修正したいロジックとそれの証明を書いたつもりになれたので
つよい気持ちでPRを送りました。

結果

コード提出から、1日程度で承認されマージされました。
次のAnsible2.8から、この内容が入ると思われます。
やっぱり、普段使っているツールに自分のソースコードが入ると嬉しいものですね。

また、レビュー時にはこのようなコメントを貰いました。

レビューされた時に、ポジティブフィードバックを貰えるとすごく嬉しくなるな、というのを再実感しました。
ちゃんと地道に必要な事を行えば、OSSのコントリビュート自体はできるな、と思ったので
OSSが更に活発になれば（そしてあわよくばAnsibleのCloudモジュールがもっと活発になれば）嬉しいですね。
Ansibleのコントリビュートとかを考えている方がいて、何かの参考になれば嬉しいです。

裏話

ここまで、やりきれた綺麗な内容しか書いて無いのですが、この裏では多くの失敗がありました。
https://github.com/ansible/ansible/pull/37820
最初に私が出したPRはひどいものでした。
Sanityテストも行わずに、説明も不足。
結果、多くのソースコードレビューをしてもらったのに
最終的に同じような機能が別のモジュールでできるという事でマージされませんでした。
（それは正しい事だと感じているので、このPRは私がクローズしています）

また、次のPRも精度がよくありませんでした。
https://github.com/ansible/ansible/pull/39002
テストコードを追加していないので、コードを追加しても「これは正しいのか？」というのが
第三者からすると非常にわかりにくいのです。
今だからこそ、わかるのですが。
ソースコードのレビューだけだと、レビュアーがどのように優れていても判断に時間がかかったりすることもあり
結果コードのマージまで時間がかかってしまうのかな、と思います。
実際このPRでは、3週間程度マージまで時間がかかっていますね。

自分の書いたコードを一番理解出来るのは、自分自身のハズなので
OSSなどでは、如何に 他人から見て自分が何をしたいかわかる状態を作るか が大事だな、と思いました。
テストコードは、その一例でしか無いとは思うのですが、結果的にはより良くなったな、と思いました。

最初は、英語で全く知らない海外のエンジニアとコミュニケーションなんて怖い、と思うかもしれません。
ただ、どのような内容も普段の技術の延長にあるはずなので、しっかりと出し切れば大抵の事はなんとかなると思います。
また、レビューにこけても諦めないことが大事かな、とも感じるので
立ち返って修正する所を修正して、次につなげていくような習慣が出来ればいいなと思うのでした。

編集履歴

2018/12/16
日本語が乱れていたので、大幅に修正。