Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rst-reader back again #6619

Merged
merged 10 commits into from Jun 25, 2019

Conversation

@raskchanky
Copy link
Member

commented Jun 4, 2019

This reverts #6616 which reverted the addition of the rst-reader tool. In addition to that reversion, it also accounts for the case where a supervisor is started up fresh for the first time, and no rumor file exists yet. Previously, the supervisor would panic, which was sub-optimal. Now we just create an empty file if we need to.

Resolves #3169

@chef-expeditor

This comment has been minimized.

Copy link

commented Jun 4, 2019

Hello raskchanky! Thanks for the pull request!

Here is what will happen next:

  1. Your PR will be reviewed by the maintainers.
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@mwrock

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

I'm wondering if you rebased if the clippy errors go away.

@raskchanky

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

@mwrock It's already been rebased against master. I tried again just now and it's up to date. I think the rust 1.35.0 PR has some fixes as well that will help.

.read(true)
.write(true)
.open(&data_path)
.map_err(|err| Error::DatFileIO(data_path.clone(), err))?;

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member

I think this is going to potentially break the atomicity of writing this file. Especially now that we potentially have multiple processes accessing it, it's important that we access this safely.

I think we could re-work this a bit more such that

  1. If the supervisor calls this and there's no existing .rst file, it's created via the standard DatFile::write method, which will use AtomicWriter
  2. If rst-reader calls this and there's no existing .rst file, it simply returns None/Err; I think an empty file vs no file is an important distinction.

Probably splitting this into two functions DatFile::read_or_create(data_path: PathBuf, server: &Server) -> Result<Self> and DatFile::read() -> Option<Self> for those two cases would make it more ergonomic. This would also eliminate the need for the if size == 0 check in read_into.

I'm also a little concerned that the the abstraction here is still a bit off since we have calls like

dat_file.read_rumors::<Service>(dat_file.service_len())

Consumers needing to tell dat_file exactly the offset to read at seems like a fairly big hole in the encapsulation. This is a problem with the initial implementation, not your additions. I think it would be good to keep re-working it a bit more so we can get at something more usable.

This comment has been minimized.

Copy link
@raskchanky

raskchanky Jun 6, 2019

Author Member

Previous to the addition of rst-reader, the only place that DatFile::new() was called was within the butterfly server when it started up. Would giving rst-reader its own method to use, e.g. DatFile::read() as you suggested, solve the concerns around creating the file atomically? I realize that doesn't provide any type of atomicity, per se, but it does resolve the issue of multiple processes trying to create the file.

I bring it up because DatFile::write takes self, which implies a certain chicken/egg problem when using it inside an initializer. I tried changing DatFile::write to be a static method, but then the call site in the butterfly server ends up looking like DatFile::write(dat_file.path(), self), which I dunno...feels a little odd.

Is there an ergonomic way to make this work that I'm missing?

This comment has been minimized.

Copy link
@raskchanky

raskchanky Jun 6, 2019

Author Member

Nevermind, I think I have a solution to this. Also, your point about consumers needing to tell dat_file the offset is taken. That also felt off to me. I'll update this to account for that.

@raskchanky raskchanky force-pushed the jb/add-rst-reader-again branch from 2a5c0ab to 9825082 Jun 6, 2019

@baumanj
Copy link
Member

left a comment

Stopping my review since we may be changing direction here, but will still share what I've got so far since I think it could be useful.

reader }))
} else {
None
}

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member

I think this can be simplified:

    pub fn read(data_path: &Path) -> io::Result<Self> {
        Ok(DatFile { header:      Default::default(),
                     header_size: Default::default(),
                     path:        data_path.to_path_buf(),
                     reader:      BufReader::new(File::open(&data_path)?), })
    }

which, with the corresponding simplifications in main, gives nice, usable errors:

➤ cargo run -p habitat-rst-reader non-existent-file.rst
[2019-06-06T19:57:59Z ERROR rst_reader] Could not read dat file non-existent-file.rst: No such file or directory (os error 2)
➤ cargo run -p habitat-rst-reader insufficient-perms.rst 
[2019-06-06T20:00:04Z ERROR rst_reader] Could not read dat file insufficient-perms.rst: Permission denied (os error 13)

This comment has been minimized.

Copy link
@raskchanky

raskchanky Jun 21, 2019

Author Member

Nice, that's much better.

error!("Dat file at {} does not exist.", path.display());
process::exit(1);
}
};

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member

With the above changes to DatFile::read, the code from L39–50, can be simplified to:

    let dat_file = dat_file::DatFile::read(Path::new(file)).unwrap_or_else(|e| {
                                                               error!("Could not read dat file \
                                                                       {}: {}",
                                                                      file, e);
                                                               process::exit(1);
                                                           });

This also adds in the provided path and uses Display rather than Debug to make

[2019-06-06T19:54:12Z ERROR rst_reader] Could not read dat file: Os { code: 2, kind: NotFound, message: "No such file or directory" }

into the friendlier:

[2019-06-06T19:55:44Z ERROR rst_reader] Could not read dat file ../habitat/acceptance.rs: No such file or directory (os error 2)

This comment has been minimized.

Copy link
@raskchanky

raskchanky Jun 21, 2019

Author Member

Excellent


let dat_path = path.join(format!("{}.rst", &self.member_id));
let mut file = DatFile::read_or_create(dat_path, &self)?;

if file.path().exists() {

This comment has been minimized.

Copy link
@baumanj

baumanj Jun 6, 2019

Member

This check should no longer be necessary. If DatFile::read_or_create does't return Ok(…), we'll early-exit.

This comment has been minimized.

Copy link
@raskchanky

raskchanky Jun 21, 2019

Author Member

Ah, good call.

@raskchanky raskchanky force-pushed the jb/add-rst-reader-again branch 2 times, most recently from 27716ab to 1e3c79d Jun 20, 2019

Add rst-reader back in.
This reverts 94156b3 as well as makes
various improvements on top of that reversion. In particular, DatFile
now has two separate initializers: one for the butterfly Server, which
will need to create the file if it doesn't already exist, and one for
rst-reader (or similar utilities) that should have read-only access, and
shouldn't be creating this file if it doesn't exist.

Signed-off-by: Josh Black <raskchanky@gmail.com>

@raskchanky raskchanky force-pushed the jb/add-rst-reader-again branch from 1e3c79d to 0f6e60a Jun 21, 2019

Fix the broken test
Signed-off-by: Josh Black <raskchanky@gmail.com>
@raskchanky

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

@baumanj I believe I've addressed all your feedback. I also rebased this onto master, and there were a number of conflicts with the work you did in #6662. I believe I've sorted those out correctly (everything compiles and appears to work) but would appreciate another look through this when you get the chance.

raskchanky added some commits Jun 21, 2019

Allow clippy::identity_conversion due to clippy bug
Signed-off-by: Josh Black <raskchanky@gmail.com>
Fix a few stray items in CI
Signed-off-by: Josh Black <raskchanky@gmail.com>

@raskchanky raskchanky force-pushed the jb/add-rst-reader-again branch from 003013e to 1c3cab1 Jun 21, 2019

@baumanj
Copy link
Member

left a comment

Looks good. Just a few optional thoughts on making the DatFile a little more robust.

components/butterfly/src/rumor/dat_file.rs Outdated Show resolved Hide resolved
components/butterfly/src/rumor/dat_file.rs Outdated Show resolved Hide resolved
components/butterfly/src/rumor/dat_file.rs Outdated Show resolved Hide resolved

raskchanky added some commits Jun 24, 2019

Make the header version constants more self-explanatory
Signed-off-by: Josh Black <raskchanky@gmail.com>
Remove the need for clients to know about header versions
Signed-off-by: Josh Black <raskchanky@gmail.com>
Be a bit more explicit about which header versions we're reading
Signed-off-by: Josh Black <raskchanky@gmail.com>
Fix test to account for stored version
Signed-off-by: Josh Black <raskchanky@gmail.com>
Make the Header private
Signed-off-by: Josh Black <raskchanky@gmail.com>
Catch a corrupt file and abort
Signed-off-by: Josh Black <raskchanky@gmail.com>
@raskchanky

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

It's worth noting that in the course of addressing PR feedback, I uncovered an issue where previously, if butterfly tried to start up and the rumor file was corrupt, it would silently succeed. With the changes I've made, that's no longer the case. Now if DatFile detects that the file is corrupt, it will abort with an error.

@raskchanky raskchanky merged commit cd269e9 into master Jun 25, 2019

5 checks passed

DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #2490 passed (1 hour, 47 minutes, 42 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #2982 passed (32 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

@raskchanky raskchanky deleted the jb/add-rst-reader-again branch Jun 25, 2019

chef-ci added a commit that referenced this pull request Jun 25, 2019

Update CHANGELOG.md with details from pull request #6619
Obvious fix; these changes are the result of automation not creative thinking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.