Force files existing on a target location to be removed when deploying with fileExistsBehavior as "OVERWRITE"

[americanomin]

Issue aws#143:

Summary

Fixed an error that caused the OVERWRITE option to behave abnormally if the source file is a symlink and the file already exists in the destination path. ( raise Errno::EEXIST Exception )

Description of changes:

This PR makes the installer deletes the destination files before copying the files If using the OVERWIRTE option.

Reason

1. If a file already exists, the installer should not call the CopyCommand class.
   • So I changed the call to CopyCommand only if there were no files.
2. If the Destination file is a directory, must prevent the file from being copied to the sub-directory.
   • Errno::EEXIST Exception can be avoided by adding the force=> true option to FileUtils.symlink. ( in CopyCommand.execution() function )
   • However, if the Destination file is a directory, the symlink file is copied below the directory (the problem is also occurring with FileUtils.copy).

issue error log sample

2019-01-05 10:29:24 ERROR [codedeploy-agent(16431)]: InstanceAgent::Plugins::CodeDeployPlugin::CommandPoller: Error during perform: Errno::EEXIST - File exists @ syserr_fail2_in - /tmp/apps/app/sym-folder/apps - /usr/lib/ruby/2.3.0/fileutils.rb:358:in `symlink'
/usr/lib/ruby/2.3.0/fileutils.rb:358:in `block in ln_s'
/usr/lib/ruby/2.3.0/fileutils.rb:1585:in `fu_each_src_dest0'
/usr/lib/ruby/2.3.0/fileutils.rb:356:in `ln_s'

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[spilist]

코멘트 몇 개 남깁니다.

1. 문제가 "symlink가 overwrite 안된다"는 건데, 에러 로그는 mkdir에서 났다고 나오는 게 왜 그런건가요?
2. FileUtils.rm_rf(destination)을 추가해서 해결하셨는데,
   • normal copy는 non-directory 파일에 대해서만 불리는 것 아닌가요? 그러면 recursive 옵션은 필요없을 것 같은데 맞나요? (그냥 remove면 충분하지 않을까)
   • 이렇게 하면 symlink만 overwrite되는게 아니라 모든 non-directory 파일에 대해서 변경이 적용되는 건데 이게 의도하신 바가 맞나요?
3. 관련된 테스트도 추가되면 더 좋을 것 같습니다.

[americanomin]

이전 답변에 오류가 있어서, 다시 코멘트 남깁니다~

문제가 "symlink가 overwrite 안된다"는 건데, 에러 로그는 mkdir에서 났다고 나오는 게 왜 그런건가요?

다시 확인해보니, 제가 잘못된 로그를 첨부했었네요.😟
실제 오류 로그는 다음과 같습니다.

2019-01-05 10:29:24 ERROR [codedeploy-agent(16431)]: InstanceAgent::Plugins::CodeDeployPlugin::CommandPoller: Error during perform: Errno::EEXIST - File exists @ syserr_fail2_in - /tmp/apps/app/sym-folder/apps - /usr/lib/ruby/2.3.0/fileutils.rb:358:in `symlink'
/usr/lib/ruby/2.3.0/fileutils.rb:358:in `block in ln_s'
/usr/lib/ruby/2.3.0/fileutils.rb:1585:in `fu_each_src_dest0'
/usr/lib/ruby/2.3.0/fileutils.rb:356:in `ln_s'

normal copy는 non-directory 파일에 대해서만 불리는 것 아닌가요?

generate_normal_copy는 source가 non-directory인 경우에만 호출됩니다.
그래서 source가 non-directory이고, destination이 directory인 경우에는 호출될 수 있습니다.

symlink만 overwrite되는게 아니라 모든 non-directory 파일에 대해서 변경이 적용되는 건데 이게 의도하신 바가 맞나요?

의도한바가 맞습니다.

• OVERWRITE 옵션을 사용하는 경우, 파일 포맷 상관없이 OVERWRITE 되어야합니다.
  기존에는 non-directory인 경우, CopyCommand에서 FileUtils.copy 함수가 암묵적으로 파일을 덮어써줬습니다.

• destination이 이미 존재하고, 디렉토리인 경우, FileUtils.symlink / FileUtils.copy 모두 디렉토리 하위에 파일을 복사합니다.
  이를 방지하기 위해, CopyCommand를 호출하기 전에 파일을 모두 지워줘야한다고 생각했습니다.

테스트코드

현재 테스트코드 구조로는 오류 현상을 재현하기 어려워서, 생략하려고 합니다.

• installer_test.rb는 CopyCommand 객체를 모두 mocking 처리하고 있습니다. 그래서 테스트코드 내에서 실제로 파일을 조작하는 함수들이 호출되지 않습니다.
  generate_normal_copy에서 넘겨주는 매개변수에 따라, CopyCommand 안에서 오류가 발생하는 건데, 현재 구조로는 installer_test.rb에 테스트를 추가하기가 쉽지않네요.
• 그렇다고 generate_normal_copy 함수에서 overwirte인 경우 rm_rf를 호출하는지를 확인하는건, 기능과 테스트코드가 동일해지기 때문에 지양해야한다고 생각합니다.

[spilist]

저라면 이렇게 쓸 것 같다, 는 느낌으로 써봅니다.

제목: 이슈 넘버는 PR 맨 위에 쓰고, 제목에는 안씁니다. 제목은 이 PR을 적용하면 결과적으로 어떤 일이 벌어지는지 구체적으로 요약해서 적습니다.

• Fixes CodeDeploy agent does not use force option of ruby symlink filetools aws/aws-codedeploy-agent#143. Fix OVERWRITE option bug when source file is symlink and destination file exists
• Force files existing on a target location to be removed when deploying with fileExistsBehavior as "OVERWRITE"

---

(여기부터 본문)

This closes aws#143.

TL;DR

(한줄 요약을 영어로 보통 이렇게 씁니다. 참고)

When deploying, rm -rf all existing files in a target location if OVERWRITE option is set.

A long story why OVERWRITE option doesn't work correctly

When creating a deployment, codedeploy handles files that already exist in a deployment target location but weren't part of the previous successful deployment with fileExistsBehavior option. If the option is OVERWRITE, the deployment should replace files in the target location to the source files.

However, OVERWRITE doesn't work as expected when some conditions are met.

Suppose my source file is source and destination is target_dir/source. Since I set fileExistsBehavior as OVERWRITE, I want to have a result of target_dir/source regardless of existence of the destination file. But if target_dir/source already exists, only 1 out of 4 cases works correctly.

• When the source file is not a symbolic link, the file is copied with FileUtils.copy.
  • (1) ⭕️target_dir/source as a regular file → result: target_dir/source, overwritten.
  • (2) ❌target_dir/source as a directory → result: target_dir/source/source.
• When the source file is a symbolic link, the file is copied with FileUtils.symlink.
  • (3) ❌target_dir/source as a regular file → result: Errno::EEXIST error occurs, and the deployment fails.
  • (4) ❌target_dir/source as a directory → result: target_dir/source/source.

That is, only case (1) is accidentally handled without a problem due to default behavior of FileUtils.copy. But since other cases are rare, it has not been a big problem.

I assume that the time when OVERWRITE feature is updated, the corresponding updates of Copy command for already existing files were missed. The evidence is the following comments in CopyCommand#execute of install_instructions.rb.

NO need to check if file already exists in here, because if that's the case,
the CopyCommand entry should not even be created by Installer

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[spilist]

저는 테스트는 "generate_normal_copy 함수에서 overwirte인 경우 rm_rf를 호출하는지를 확인"하는 것이면 충분하다고 생각합니다. 기능과 테스트가 동일하다고 하셨는데 그게 문제가 될까요?

[americanomin]

@spilist
Mock으로 행동을 검증하는 방식을 사용해서, 구현부와 테스트코드 간의 커플링이 지나치게 높아지는건 지양해야한다고 생각합니다.

이렇게 생각하는 이유는 크게 두가지입니다.

• 테스트코드의 유연성이 떨어짐
  • 구현부에서 파일을 삭제하는 함수를 변경하는 경우, 테스트 코드가 깨지게 됨.
• 깨질 수 없는 당연한 논리를 검증하는 거라고 여겨짐
  • "A 함수를 호출했더니, A 함수가 호출되었다"라는 당연한 논리를 검증하는걸로 여겨짐.

이미 installer_test.rb는 Mockist를 기반으로 테스트를 검증하고 있는데, ( ex. CopyCommand.copy 호출여부 검증 ) rm_rf를 Mockist으로 테스트하는 건 왜이리 찜찜한건지 제 생각을 더 정리해봐야겠어요. ㅎㅎ

[spilist]

저는 "OVERWRITE일 때 destination에 대해 FileUtils.rm_rf가 호출된다"가 그리 직관적이지는 않기 때문에, "A 함수를 호출했더니, A 함수가 호출되었다"와는 다르다고 생각하긴 합니다. 하지만 installer test에서 의도하는 게 어쨌든 "OVERWRITE일 때 파일의 기존 존재 유무와 상관없이 카피가 잘 된다"를 테스트하는 것이라고 본다면 너무 구현 디테일에 의존성이 커지는 건 맞는 것 같아요.

문득 생각난 게 있는데 이런 식으로 구현을 바꾼다면 어떨까요?

# installer.rb
def generate_normal_copy(i, absolute_source_path, destination)
...
  when "OVERWRITE"
    i.copy(absolute_source_path, destination, overwrite: true)
...

# install_instruction.rb
def copy(source, destination, overwrite: false)
  destination = sanitize_dir_path(destination)
  FileUtils.rm_rf(destination) if overwrite
  ...
end

(보니까 sanitize_dir_path를 command execute하기 전에 해줘서 절대경로로 바꾸더군요. 근데 어차피 앱스펙에 절대경로로 써두지 않나? 아무튼..)
구현을 이렇게 바꾸면 installer test에서는 copy command를 overwrite: true 옵션과 함께 부르는지 테스트하면 되고, install_instruction test에서는 overwrite 옵션이 있을 때 FileUtils.rm_rf가 불리는지 테스트하면 될 것 같아요. 좀더 concern이 분리되는 쪽으로 생각하면 이쪽인 것 같은데 어떠세요?

@instruction_builder
              .expects(:copy)
              .with("deploy-archive-dir/src1", "dst1/src1", overwrite: true)

[wonderer80]

• So I changed the call to CopyCommand only if there were no files.

이게 무슨 말인지 잘 모르겠어요. 변경사항은 그냥 rm_rf 만 추가된 것 같은데요.

[wonderer80]

저는 대상이 directory 일 때 무조건 rm_rf 하는 것은 좀 위험하다고 생각이 드는데요.
배포할 때 cleanup_file 까지 작성해가며 배포파일과 아닌 것을 구분하며 관리를 해주는데 디렉토리를 통째로 지운다는게 위험하게 느껴지거든요. 최소한 디렉토리안에 존재하는 파일이 없을 때에만 가능하게 해야할 것 같아요.
(음?위의 내용이 그 말이었나?ㅎㅎ 근데 rm_rf 는 파일이 있던 없던 지우는걸텐데요..)

[americanomin]

@wonderer80

So I changed the call to CopyCommand only if there were no files.

이 문장은 "(CopyCommand를 호출하기 전에, 파일을 삭제함으로써) 파일이 없는 경우에만 CopyCommand를 호출하도록 변경했다"는 것을 (엄청..ㅎㅎ..) 함축한거였습니다.

복사 경로에 파일이 존재하지 않는 경우만, CopyCommand를 호출해야한다는 주석을 고려했다는걸 알리고 싶어서, 이런 코멘트를 추가했었습니다.

휘동님이 정리해주신 풀리퀘스트에 제가 전달하고 싶은 내용이 포함되어 있어서, 위의 코멘트가 지워져도 좋을 것 같아요.

The evidence is the following comments in CopyCommand#execute of install_instructions.rb.

NO need to check if file already exists in here, because if that's the case,
the CopyCommand entry should not even be created by Installer

---

그리고 OVERWRITE 옵션은 무조건 파일을 덮어쓰는 기능이라고 생각하고 있는데요.
그래서 배포 파일이 아닌 파일이 삭제되는게 우려된다면, OVERWRITE 옵션을 사용하지 말아야한다고 생각해요.

non-directory 파일을 복사하려는데, 대상경로에 directory가 있고, 그 안에 파일이 존재하는 경우에는 오류를 리턴시킨다는건, OVERWRITE 목적과는 다르단 생각이 드네요.

기존 디렉토리가 삭제되는게 우려된다면,
배포 파일이 아닌 파일이 존재하는 경우 오류를 내뱉는 디폴트 설정을 사용하거나,
DISALLOW / RETAIN 옵션을 사용하는게 옳다고 생각합니다.

[americanomin]

@spilist
rm_rf 함수 위치를 CommandBuilder 쪽으로 변경하는게, 관심사 분리 / 재사용성 측면에서 더 좋을 것 같군요.
installer test 의도도 명확해지는 것 같아요.

이상하게도, generate_normal_copy나 CopyCommmand에서만 OVERWRITE 기능을 다뤄야한다고 생각했는데, CommandBuilder > copy가 더 적합한 위치겠네요.

[wonderer80]

저는 좀 다르게 생각하는데요. 기존에 위치한 것이 디렉토리인데 새로 배포되는 동일한 이름은 파일이라면 의도한 것이 아닐 가능성이 높다고 생각합니다.

그리고 일반적으로 OVERWRITE라는 용어가 file에서 file을 복사할 때 덮어씌우는 것을 의미하기 때문에 동일한 디렉토리가 있으면 지우고 파일로 대체하는 것을 기대하는 사람은 없다고 생각해요.

linux에서도 cp명령어로 동일한 상황을 테스트해보았을 때의 경우도 살펴보면

file to file 이면 overwrite를 하겠는지 물어보고 y를 누르면 덮어씌워지지만,
file to directory이면 overwrite를 하겠는지 물어보지만 y를 눌러도 아래와 같은 메시지가 나오고 덮어씌우지 않습니다.

cp: cannot overwrite directory ‘test/appspec.yml’ with non-directory

[spilist]

일반적으로 OVERWRITE라는 용어가 file에서 file을 복사할 때 덮어씌우는 것을 의미하기 때문에 동일한 디렉토리가 있으면 지우고 파일로 대체하는 것을 기대하는 사람은 없다고 생각해요.

사실 저도 이부분이 찝찝하긴 했었어요. 복사하려는 파일과 동일한 이름의 디렉토리가 존재하는 상황 자체가 비정상이라고 보면 이 경우에는 exception이 생기는게 더 나을지도 모르겠네요.

[americanomin]

@wonderer80
cp는 복사대상경로가 디렉토리이면, 디렉토리안에 파일을 복사하고 있는데요.

OVERWRITE 옵션을 사용할 때, 이 행동을 기대하진 않을 것 같아요.

그래서 이를 방어하는 방법으로 폴더를 삭제하는 방식을 사용했습니다.

그런데 명시적으로 오류를 리턴하는 방식으로 위의 상황을 막는게 좋겠군요.

[americanomin]

파일을 삭제하는 로직을 추가했던건, Destination이 디렉토리인 경우 디렉토리 하위에 파일이 복사되는 현상을 막기 위함이였습니다.

그런데 non directory인 경우에만, CopyCommand가 호출된다면,
symlink에 force 옵션을 주는 것으로도 문제를 해결할 수 있어서, rm_rf 호출 로직을 제거해뒀습니다 👍

[americanomin]

TL;DR

when deploying with fileExistsBehavior as "OVERWRITE" AND target location is not regular file ( symlink or directory ), Fix errors that occur.

OVERWRITE 옵션을 사용하고 있고, 타겟 경로에 디렉토리 혹은 Symlink 파일이 존재하는 경우에 발생하는 오류를 수정했습니다.

A long story why OVERWRITE option doesn't work correctly

When creating a deployment, codedeploy handles files that already exist in a deployment target location but weren't part of the previous successful deployment with fileExistsBehavior option. If the option is OVERWRITE, the deployment should replace files in the target location to the source files.

However, OVERWRITE doesn't work as expected when some conditions are met.

Suppose my source file is source and destination is target_dir/source. Since I set fileExistsBehavior as OVERWRITE, I want to have a result of target_dir/source regardless of existence of the destination file. But if target_dir/source already exists, only 1 out of 4 cases works correctly.

• When the source file is not a symbolic link, the file is copied with FileUtils.copy.
  • (1) ⭕️target_dir/source as a regular file → result: target_dir/source, overwritten.
  • (2) ❌target_dir/source as a directory → result: target_dir/source/source.
• When the source file is a symbolic link, the file is copied with FileUtils.symlink.
  • (3) ❌target_dir/source as a regular file → result: Errno::EEXIST error occurs, and the deployment fails.
  • (4) ❌target_dir/source as a directory → result: target_dir/source/source.
    That is, only case (1) is accidentally handled without a problem due to default behavior of FileUtils.copy. But since other cases are rare, it has not been a big problem.

If the target location was a directory (2, 4 Case), I didn't think it was the intended situation. so in this instance, explicitly exception was generated.

배포하는 파일은 regular 파일인데, 타겟 파일은 디렉토리인 경우는 의도한 상황이 아니라고 생각했다. 그래서 이 경우, OVERWRITE 옵션을 사용했음에도 오류를 리턴하게 변경했다.