티스토리 뷰

Cloud Computing/Openstack

[OpenStack에 contribute하기 - 3] Git (2)

최성훈 2019. 12. 29. 00:29
반응형

본 포스트에서는 OpenStack에 contribute 할 커밋들을 작성하는 좋은 예시와 나쁜 예시를 알아볼 것입니다.

공식 문서를 번역하는 것만으로도 도움이 되는 것을 느낍니다.

 

Git Commit의 좋은 관행

개요

이 글에서 제기될 핵심과 예제들은 변경 사항을 개별적인 커밋들로 분할하는 것에 대한 가치와, 좋은 커밋 메시지들을 작성하는 중요성에 대해서 증명할 것입니다. 만약 이 가이드라인들이 광범위하게 적용된다면 OpenStack Git 기록들의 질을 향상하는 데에 엄청난 기여를 할 것입니다. 이 글은 사람들에게 여러 혜택들을 인식하게 하는 당근과 같은 반면에, Gerrit code를 리뷰하는 누구나 채찍이 될 수 있습니다.

바꿔 말하면, Gerrit에서 변경사항들을 리뷰할 때, 코드의 정확성에 대해서만 간단하게 들여다보지 말라는 뜻입니다.

즉,

  • 커밋 메시지 자체를 리뷰하고, 커밋 메시지의 자체로 개선을 요구하세요.
  • 여러 논리적인 변화가 섞이는 커밋들을 조심하고, 커미터(commiter)에게 논리적으로 섞여있는 커밋들을 개별 커밋들로 분할하라고 요청하세요.
  • 기능적 변경사항과 함께 공백 문자의 변경사항이 섞이지 않도록 하세요.
  • 여러 기능적 변경사항들로부터 개별적으로 no-op 코드 리팩터링을 하지 마세요. (코드 리팩터링은 한 번에 하라는 뜻인 것 같습니다.)
  • 기타 등등...

소프트웨어 소스코드는 "많이 읽히고, 드물게 작성"됩니다. 따라서 가장 중요한 기준은 커뮤니티 내의 커다란 개발자 그룹에 의해서 장기적인 유지보수성(maintainability)을 향상시키는 것이지, 코드를 두 번 다시 안 볼 수도 있는 한 명의 개발자를 위해서 희생을 하는 것이 아닙니다.

 

변경사항의 구조적 분할

좋은 커밋을 만들기 위한 기본적인 규칙은 커밋당 하나의 "논리적 변화"만 확실하게 하는 것입니다. 이것이 중요한 규칙인 이유는 다음과 같습니다.

  • 변경되는 코드의 양이 적을수록 잠재적인 결함을 검토하고 식별하는 게 더 빠르고 쉽습니다.
  • 만약 변경사항이 나중에 결함으로 발견되면, 잘못된 커밋을 되돌리는 것이 필요할 수도 있습니다. 이것은 원래의 커밋과 얽혀있는, 다른 관련 없는 커밋들이 없다면 훨씬 더 쉽게 할 수 있습니다.
  • Git의 이등분적인 성질을 사용해서 문제를 해결할 때, 잘 정의된 작은 변화는 문제가 되는 코드를 도입한 위치를 분리하는데 도움이 될 것입니다.

커밋을 작성할 때 피해야 하는 것들

  • 공백 문자의 변경사항과 코드 변경사항의 혼합
    공백 문자의 변경은 기능적 변경사항을 모호하게 해서 리뷰어가 해당 변경이 올바른지 정확하게 판단하기 어렵게 합니다. 해결법: 공백 문자의 변경사항, 기능적 변경사항으로 2개의 커밋을 생성합니다. 일반적으로는 공백 문자의 변경이 먼저 이루어지지만 엄격한 규칙은 아닙니다.
  • 두 개의 관련 없는 기능적 변경사항들의 혼합
    다시, 만약 관련 없는 두 변경사항들이 혼합되면 리뷰어들이 문제를 식별하기 어려울 것입니다. 그리고 만약 나중에서야 잘못된 커밋을 되돌려야 한다면, 관련 없는 변경 사항들은 분리되어야 할 것이고, 이는 또 다른 버그의 위험이 있습니다.
  • 한 개의 커밋 속 커다란 새로운 기능
    이는 하나의 기능 전체가 하나의 커밋으로 제공되면 안 된다는 것을 의미합니다. 새로운 기능은 종종 기존의 코드를 리팩터링 해야 합니다. 모든 리팩터링은 새로운 기능을 구현하는 작업과는 별개의 작업으로 수행하는 것이 매우 바람직합니다. 이는 리뷰어와 테스트 셋들이 리팩터링에 의해 의도하지 않은 변화가 없다는 것을 검증하는데 도움이 됩니다. 

잘못된 구조적 분할에 대한 예

Nova 기록에 대한 예시를 들어봅시다. 집중해주세요, 커밋 해쉬가 참조용으로 인용됐지만, 작성자 이름은 지웠습니다. 왜냐하면 어떤 사람도 비난받을 필요가 없기 때문입니다. 거의 모든 사람들이 언젠가 이런 좋은 관행들을 어긴 것에 대한 잘못이 있습니다. 게다가 이런 커밋을 리뷰하고 승인한 사람들도 이러한 커밋을 작성하고 제출한 사람만큼의 잘못이 있습니다.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
  commit ae878fc8b9761d099a4145617e4a48cbeb390623
  Author: [removed]
  Date:   Fri Jun 1 01:44:02 2012 +0000
 
    Refactor libvirt create calls
 
     * minimizes duplicated code for create
     * makes wait_for_destroy happen on shutdown instead of undefine
     * allows for destruction of an instance while leaving the domain
     * uses reset for hard reboot instead of create/destroy
     * makes resume_host_state use new methods instead of hard_reboot
     * makes rescue/unrescue not use hard reboot to recreate domain
 
    Change-Id: I2072f93ad6c889d534b04009671147af653048e7
 cs

최소 2개의 개별적인 변경사항이 이 커밋 안에 있습니다.

  1. "hard_reboot" 메소드 대신 새로운 "reset" API의 사용 전환
  2. 내부 드라이버들이 "hard_reboot"를 사용하지 않도록 조정

이것과 관련한 문제는 다음과 같습니다.

  • 첫 번째로, 이러한 변화가 동시에 이루어져야 하는 설득력 있는 이유가 없습니다. 첫 번째 커밋은 여러 곳에서 "hard_reboot"를 호출하는 것을 멈추는 변경사항을 포함할 수 있었습니다. 두 번째 커밋은 "hard_reboot" 구현을 다시 작성할 수 있었습니다.
  • 두 번째로, libvert의 'reset' 메소드를 사용하는 변경이 대규모 코드 리팩터링에 묻히면서 리뷰어들이 더 새로운 libvirt API 버전에 의존성이 있다는 사실을 놓쳤습니다. 이 커밋이 문제가 되는 원인이라는 것이 빠르게 확인되었지만, 관련 없는 다양한 변경사항이 포함되어 있기 때문에 사소한 복구는 불가능합니다.

좋은 구조적 분할에 대한 예

1
2
3
4
5
6
7
8
9
10
11
  commit 3114a97ba188895daff4a3d337b2c73855d4632d
  Author: [removed]
  Date:   Mon Jun 11 17:16:10 2012 +0100
 
    Update default policies for KVM guest PIT & RTC timers
 
  commit 573ada525b8a7384398a8d7d5f094f343555df56
  Author: [removed]
  Date:   Tue May 1 17:09:32 2012 +0100
 
    Add support for configuring libvirt VM clock and timers
 cs

이 두 가지 변경 사항을 함께 적용하면 KVM 게스트 타이머를 구성할 수 있습니다. libvirt XML 구성을 생성하기 위한 새로운 API의 도입은 그 API를 사용하는 KVM 게스트 생성 정책의 변경 사항과 명확하게 분리되어 있습니다.

 

커밋 메시지 속 정보

변경 사항의 내용만큼이나 중요한 것은 그것을 설명하는 커밋 메시지의 내용입니다. 커밋 메시지를 쓸 때 기억해야 할 몇 가지 중요한 사항이 있습니다.

  • 리뷰어가 원래 문제가 무엇인지 이해한다고 가정하지 마세요.
  • 리뷰어가 외부 웹 서비스/사이트에 액세스 할 수 있다고 가정하지 마세요.
  • 코드가 그 자체로써 증명/문서화한다고 가정하지 마세요.
  • 이 변경사항이 왜 생기는지 설명하세요.
  • 개선된 코드에 힌트가 있는지 커밋 메시지를 읽으세요.
  • 리뷰를 결정하기에 충분한 정보가 있는지 확인하세요.
  • 첫 번째 커밋 줄이 가장 중요합니다.
  • 현재 코드의 제약 사항에 대해서 설명하세요.

따라야 할 핵심 규칙은 다음과 같습니다.

커밋 메시지는 패치의 정확성을 완전히 이해하고 검토하는 데 필요한 모든 정보를 포함해야 합니다. 적을수록 더 좋은 게 아닙니다. 많을수록 더 좋습니다.

 

잘못된 관행에 대한 예

예시 1)

1
2
3
4
5
6
7
    commit 468e64d019f51d364afb30b0eed2ad09483e0b98
    Author: [removed]
    Date:   Mon Jun 18 16:07:37 2012 -0400
 
      Fix missing import in compute/utils.py
 
      Fixes bug 1014829
 cs

문제점: 이 커밋은 무엇을 import 하고, 어디가 누락이 되었으며, 왜 utils.py가 필요한 지 언급하지 않습니다. 이 정보는 실제로 버그 트래커에 있었고, 커밋 메시지 속에 복사해서 스스로 설명을 제공해야 했습니다. 예를 들면 다음과 같습니다.

1
2
3
4
      Add missing import of 'exception' in compute/utils.py
 
      nova/compute/utils.py makes a reference to exception.NotFound,
      however exception has not been imported.
 cs

 

예시 2)

1
2
3
4
5
6
7
8
9
10
   commit 2020fba6731634319a0d541168fbf45138825357
   Author: [removed]
   Date:   Fri Jun 15 11:12:45 2012 -0600
 
    Present correct ec2id format for volumes and snaps
 
    Fixes bug 1013765
    * Add template argument to ec2utils.id_to_ec2_id() calls
 
    Change-Id: I5e574f8e60d091ef8862ad814e2c8ab993daa366
 cs

문제점: 이 커밋은 현재 잘못된 포맷이 무엇인지, 새로운 포맷은 무엇인지에 대해 언급하지 않습니다. 다시 이 정보는 버그 트래커에 있었고, 커밋 메시지에 포함되어야 했습니다. 게다가 이 버그는 이전의 커밋의 변경으로 인한 회귀 현상을 고치고 있었지만, 이전의 변화가 무엇이었는지에 대한 언급이 없습니다. 예를 들면 다음과 같습니다.

1
2
3
4
5
6
7
8
9
10
    Present correct ec2id format for volumes and snaps
 
    During the volume uuid migration, done by changeset XXXXXXX,
    ec2 id formats for volumes and snapshots was dropped and is
    now using the default instance format (i-xxxxx). These need
    to be changed back to vol-xxx and snap-xxxx.
 
    Adds a template argument to ec2utils.id_to_ec2_id() calls
 
    Fixes bug 1013765
 cs

 

예시 3)

1
2
3
4
5
6
7
8
9
  commit f28731c1941e57b776b519783b0337e52e1484ab
  Author: [removed]
  Date:   Wed Jun 13 10:11:04 2012 -0400
 
    Add libvirt min version check.
 
    Fixes LP Bug #1012689.
 
    Change-Id: I91c0b7c41804b2b25026cbe672b9210c305dc29b
 cs

문제점: 이 커밋 메시지는 단순히 무엇이 행해졌는지 기록하고, 왜 행해졌는지는 기록하지 않습니다. 커밋 메시지는 이전의 어떤 변경 사항들이 새로운 최소 libvirt 버전을 도입했는지 언급해야 했습니다. 또, 검사에 실패했을 때 어떤 작동을 하는지 언급했어야 했습니다. 예를 들면 다음과 같습니다.

1
2
3
4
5
6
7
8
9
10
    Add libvirt version check, min 0.9.7
 
    The commit XXXXXXXX introduced use of the 'reset' API
    which is only available in libvirt 0.9.7 or newer. Add a check
    performed at startup of the compute server against the libvirt
    connection version. If the version check fails the compute
    service will shutdown.
 
    Fixes LP Bug #1012689.
    Change-Id: I91c0b7c41804b2b25026cbe672b9210c305dc29b
 cs

 

좋은 관행에 대한 예

예제 1)

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
  commit 3114a97ba188895daff4a3d337b2c73855d4632d
  Author: [removed]
  Date:   Mon Jun 11 17:16:10 2012 +0100
 
    Update default policies for KVM guest PIT & RTC timers
 
    The default policies for the KVM guest PIT and RTC timers
    are not very good at maintaining reliable time in guest
    operating systems. In particular Windows 7 guests will
    often crash with the default KVM timer policies, and old
    Linux guests will have very bad time drift
 
    Set the PIT such that missed ticks are injected at the
    normal rate, ie they are delayed
 
    Set the RTC such that missed ticks are injected at a
    higher rate to "catch up"
 
    This corresponds to the following libvirt XML
 
      <clock offset='utc'>
        <timer name='pit' tickpolicy='delay'/>
        <timer name='rtc' tickpolicy='catchup'/>
      </clock>
 
    And the following KVM options
 
      -no-kvm-pit-reinjection
      -rtc base=utc,driftfix=slew
 
    This should provide a default configuration that works
    acceptably for most OS types. In the future this will
    likely need to be made configurable per-guest OS type.
 
    Closes-Bug: #1011848
 
    Change-Id: Iafb0e2192b5f3c05b6395ffdfa14f86a98ce3d1f
 cs

이 예제 커밋에 대해 눈여겨봐야 할 점들은 다음과 같습니다.

  • 원래 문제가 무엇인지 설명합니다. (잘못된 KVM 기본값)
  • 적용하고자 하는 기능적 변경사항을 설명합니다. (새로운 PIT/RTC 정책)
  • 변경사항의 결과에 대해서 설명합니다. (새로운 XML/QEMU 매개변수)
  • 향후 개선의 범위를 설명합니다. (OS 유형별 구성 가능성)
  • Closes-Bug 표기를 사용합니다.

 

예제 2)

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
  commit 31336b35b4604f70150d0073d77dbf63b9bf7598
  Author: [removed]
  Date:   Wed Jun 6 22:45:25 2012 -0400
 
    Add CPU arch filter scheduler support
 
    In a mixed environment of running different CPU architecutres,
    one would not want to run an ARM instance on a X86_64 host and
    vice versa.
 
    This scheduler filter option will prevent instances running
    on a host that it is not intended for.
 
    The libvirt driver queries the guest capabilities of the
    host and stores the guest arches in the permitted_instances_types
    list in the cpu_info dict of the host.
 
    The Xen equivalent will be done later in another commit.
 
    The arch filter will compare the instance arch against
    the permitted_instances_types of a host
    and filter out invalid hosts.
 
    Also adds ARM as a valid arch to the filter.
 
    The ArchFilter is not turned on by default.
 
    Change-Id: I17bd103f00c25d6006a421252c9c8dcfd2d2c49b
 cs

이 예제 커밋에 대해 눈여겨봐야 할 점들은 다음과 같습니다.

  • 문제 시나리오가 무엇인지 설명합니다. (혼합된 구조 배치)
  • 버그 해결에 대한 의도를 설명합니다. (구조에 대한 스케줄러 필터 만들기)
  • 버그 해결에 대한 간략한 구조를 설명합니다. (libvirt가 어떻게 구조를 반환하는지)
  • 버그 해결의 한계에 대해서 기록합니다. (Xen에 필요한 작업)

 

본 포스트는 다음 주소를 참고하였습니다.

반응형
저작자표시

'Cloud Computing > Openstack' 카테고리의 다른 글

[OpenStack에 contribute하기 - 5] Gerrit 사용하기 (1)  (0) 2020.01.05
[OpenStack에 contribute하기 - 4] Gerrit Account 구성하기  (0) 2019.12.29
[OpenStack에 contribute하기 - 3] Git (1)  (0) 2019.12.28
[OpenStack에 contribute하기 - 2] Account Setup  (0) 2019.12.28
[OpenStack에 contribute하기 - 1] 개요  (0) 2019.12.28
댓글